[Mesa-dev] [PATCH] egl: fix for multiple window surfaces per single window
Ian Romanick
idr at freedesktop.org
Fri Mar 11 18:58:59 UTC 2016
Most of comments that follow are related to Mesa coding standards.
Many of these comments apply to "egl: updating surface size on x11" and
"egl: Fix for not setting EGL_BAD_MATCH by eglCreatePbufferSurface.".
I'm not sure how you're sending these patches, but you really, really
should use git-send-email. It may take a bit of effort to set up with
your mail server, but it will make life easier for everyone involved.
On 03/11/2016 01:04 AM, Pielech, Adrian wrote:
> Current implementation of eglCreateWindowSurface() is against to the EGL specification, because it allows creating multiple surfaces per single window.
Line-wrap commit messages at 72 columns.
If the code is not following the specification, include a spec
quotation either in the commit message or in the code. Spec quotations
should be formatted as:
/* Section #.#.# (section name) of the Specification Name version spec
* says:
*
* "Words from the spec."
*/
Commit 07e6a373 is a good example.
It's really hard to evaluate the correctness of a behavioral change
like this without corroboration from the spec.
On that same topic, is there a test case for this? If there is not,
please submit a piglit test that reproduces this error.
> ---
> src/egl/main/eglapi.c | 109 +++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 107 insertions(+), 2 deletions(-)
>
> diff --git a/src/egl/main/eglapi.c b/src/egl/main/eglapi.c
> index fbb14f1..3eb9fb6 100644
> --- a/src/egl/main/eglapi.c
> +++ b/src/egl/main/eglapi.c
> @@ -102,6 +102,96 @@
> #include "eglsync.h"
> #include "eglstring.h"
>
> +/**
> + * Bi direction linked list item structure for carrying window and surface fields.
> + */
^ delete the extra space here.
The * of the second and third line should line up with the first * of
the first line.
> +struct _window_list_item
Don't begin the type name with an underscore.
> +{
Opening curly brace code on the previous line. This also applies to all of the for-loops and if-statements below.
> + struct _window_list_item *prev;
> + struct _window_list_item *next;
Mesa already has several link list implementations. Please use src/util/list.h instead of open-coding.
> + EGLNativeWindowType native_window;
> + EGLSurface attached_surface;
> +};
> +
> +typedef struct _window_list_item window_list_item;
Just use the struct name. There is no reason for the extra typedef.
> +
> +struct _window_list_item *window_surface_association_list_head = NULL;
> +struct _window_list_item *window_surface_association_list_tail = NULL;
> +
> +static inline bool _isWindowAssociatedWithSurface(EGLNativeWindowType window)
The return type and the function name go on separate lines like:
static inline bool
_isWindowAssociatedWithSurface(EGLNativeWindowType window)
This enables people to find the function definition easily with 'git
grep ^function_name'
Also... name should separate words with underscores instead of
capitalization.
static inline bool
is_window_associated_with_surface(EGLNativeWindowType window)
> +{
> + struct _window_list_item *it = window_surface_association_list_head;
> + for (; it != NULL; it = it->next)
When you switch to list.h, this will become
list_for_each_entry(struct window_list_item, it, &window_surface_association_list, node) {
> + {
> + if (it->native_window == window)
> + {
> + return true;
> + }
> + }
> + return false;
> +}
> +
> +static inline void _associateWindowWithSurfaceList(EGLNativeWindowType window, EGLSurface surface)
> +{
> + window_list_item * new_list_item = malloc(sizeof(window_list_item));
^ no space here
> + memset(new_list_item, 0, sizeof(window_list_item));
Use calloc.
> +
> + new_list_item->native_window = window;
> + new_list_item->attached_surface = surface;
> +
> + if (window_surface_association_list_tail)
> + {
> + window_surface_association_list_tail->next = new_list_item;
> + new_list_item->prev = window_surface_association_list_tail;
> +
> + window_surface_association_list_tail = new_list_item;
> + }
> + else
> + {
The else goes on the previous line. So, this should be
} else {
> + window_surface_association_list_head = new_list_item;
> + window_surface_association_list_tail = new_list_item;
> + }
When you switch to list.h, the whole if-else block becomes:
list_addhead(&new_list_item->node, &window_surface_association_list);
> +}
> +
> +static inline void _disassociateWindowWithSurfaceList(EGLSurface surface)
> +{
> + if (window_surface_association_list_head != NULL)
> + {
> + if (window_surface_association_list_head == window_surface_association_list_tail &&
> + window_surface_association_list_head->attached_surface == surface)
Indentation should line up with the opening parenthesis.
> + {
> + free(window_surface_association_list_head);
> + window_surface_association_list_head = NULL;
> + window_surface_association_list_tail = NULL;
> + }
> + else
> + {
> + struct _window_list_item * it = window_surface_association_list_head;
^ no space here
> +
> + for (;it != NULL; it = it->next)
> + {
> + if (it->attached_surface == surface)
> + {
> + struct _window_list_item *next_element = it->next;
> + struct _window_list_item *prev_element = it->prev;
> +
> + free (it);
^ no space here.
> +
> + if (prev_element)
> + prev_element->next = next_element;
Blank line here.
> + if (next_element)
> + next_element->prev = prev_element;
> +
> + if (it == window_surface_association_list_tail)
> + window_surface_association_list_tail = prev_element;
> +
> + if (it == window_surface_association_list_head)
> + window_surface_association_list_head = next_element;
> + }
> + }
> + }
> + }
Once you switch to list.h, this whole function will change. You will
want to use list_for_each_entry_safe.
> +}
>
> /**
> * Macros to help return an API entrypoint.
> @@ -690,9 +780,20 @@ eglCreateWindowSurface(EGLDisplay dpy, EGLConfig config,
> EGLNativeWindowType window, const EGLint *attrib_list)
> {
> _EGLDisplay *disp = _eglLockDisplay(dpy);
> + EGLSurface window_surface = NULL;
> STATIC_ASSERT(sizeof(void*) == sizeof(window));
> - return _eglCreateWindowSurfaceCommon(disp, config, (void*) window,
> - attrib_list);
> +
> + //check if window have already created surface to it
Use /* */ style comments. Also, use proper capitalization and punctuation rules in comments. In a single-line comment the closing */ goes on the same line.
/* Check if window have already created surface to it. */
> + if (_isWindowAssociatedWithSurface(window))
> + RETURN_EGL_ERROR(disp, EGL_BAD_ALLOC, EGL_NO_SURFACE);
> +
> + window_surface = _eglCreateWindowSurfaceCommon(disp, config, (void*) window,
> + attrib_list);
> + //add window with attached surface to list,
> + //it will be freed when eglDestroySurface will be called
In a multi-line comment, the */ goes on its own line.
/* Add window with attached surface to list, and it will be freed
* when eglDestroySurface is called.
*/
> + if (window_surface != EGL_NO_SURFACE)
> + _associateWindowWithSurfaceList(window, window_surface);
> +
> + return window_surface;
> }
>
>
> @@ -805,6 +906,9 @@ eglDestroySurface(EGLDisplay dpy, EGLSurface surface)
> _eglUnlinkSurface(surf);
> ret = drv->API.DestroySurface(drv, disp, surf);
>
> + //remove surface association with window if exist
> + _disassociateWindowWithSurfaceList(surface);
> +
> RETURN_EGL_EVAL(disp, ret);
> }
>
>
More information about the mesa-dev
mailing list