Bug 98501 – [Qt] Implement GraphicsSurfaceToken to replace uint64_t as token type.

• Component: WebKit2
• Status: RESOLVED FIXED

[Zeno Albisser]

Up to now, we were using a uint64_t as a token to identify a GraphicsSurface.
For mac and linux this seemed to be straight forward and good enough, even though not perfectly well designed.
With the upcomping GraphicsSurface implementation for Windows, 64bit will not be sufficient for a token.
A token will have to contain two native HANDLES which are of type (void*). On a 64bit build we would then need 128bits.
We should therefore wrap any kind of native handles/windowid/IOSurfaceRef into a specific GraphicsSurfaceToken class.

[Zeno Albisser]

Created attachment 167294 [details]
patch for review.

[Zeno Albisser]

Created attachment 167306 [details]
patch for review. - rebased.

[kov's GTK+ EWS bot]

Comment on attachment 167306 [details]
patch for review. - rebased.

Attachment 167306 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14178454

[Early Warning System Bot]

Comment on attachment 167306 [details]
patch for review. - rebased.

Attachment 167306 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14169743

[Zeno Albisser]

Created attachment 167320 [details]
patch for review. - fixed compilation without use_graphics_surface.

[Zeno Albisser]

Comment on attachment 167320 [details]
patch for review. - fixed compilation without use_graphics_surface.

This patch will need another iteration. - I'll post an update asap.

[Zeno Albisser]

Created attachment 167331 [details]
patch for review.

[Kenneth Rohde Christiansen]

Comment on attachment 167331 [details]
patch for review.

View in context: https://bugs.webkit.org/attachment.cgi?id=167331&action=review

> Source/WebCore/ChangeLog:7
> +        This is necessary in order to implement GraphicsSurface for Windows as well.

I would remove "as well"

> Source/WebCore/ChangeLog:9
> +        WindowID (Linux/GLX), two IOSurfaceIDs (Mac) or in future two HANDLEs (Windows).

in *the* future

> Source/WebCore/platform/graphics/surfaces/GraphicsSurfaceToken.h:41
> +#if OS(LINUX)
> +    GraphicsSurfaceToken(uint32_t windowID = 0)
> +        : frontBufferHandle(windowID)
> +    { }

So this covers linux without X11?

[Pierre Rossi]

Comment on attachment 167331 [details]
patch for review.

View in context: https://bugs.webkit.org/attachment.cgi?id=167331&action=review

>> Source/WebCore/ChangeLog:9
>> +        WindowID (Linux/GLX), two IOSurfaceIDs (Mac) or in future two HANDLEs (Windows).
> 
> in *the* future

I would tend to agree, but it seems it could actually be correct according to that: http://itre.cis.upenn.edu/~myl/languagelog/archives/004201.html
Well, I'd be more comfortable with a comma before and after.
Maybe it's just us not being accustomed to british english that much...

[Zeno Albisser]

Comment on attachment 167331 [details]
patch for review.

View in context: https://bugs.webkit.org/attachment.cgi?id=167331&action=review

>> Source/WebCore/ChangeLog:7
>> +        This is necessary in order to implement GraphicsSurface for Windows as well.
> 
> I would remove "as well"

I can change that.

>>> Source/WebCore/ChangeLog:9
>>> +        WindowID (Linux/GLX), two IOSurfaceIDs (Mac) or in future two HANDLEs (Windows).
>> 
>> in *the* future
> 
> I would tend to agree, but it seems it could actually be correct according to that: http://itre.cis.upenn.edu/~myl/languagelog/archives/004201.html
> Well, I'd be more comfortable with a comma before and after.
> Maybe it's just us not being accustomed to british english that much...

And I can change that as well.

>> Source/WebCore/platform/graphics/surfaces/GraphicsSurfaceToken.h:41
>> +    { }
> 
> So this covers linux without X11?

Linux without GLX/X11 will not use GRAPHICS_SURFACE in the first place. So it's not an issue.

[Kenneth Rohde Christiansen]

Comment on attachment 167331 [details]
patch for review.

View in context: https://bugs.webkit.org/attachment.cgi?id=167331&action=review

>>> Source/WebCore/platform/graphics/surfaces/GraphicsSurfaceToken.h:41
>>> +    { }
>> 
>> So this covers linux without X11?
> 
> Linux without GLX/X11 will not use GRAPHICS_SURFACE in the first place. So it's not an issue.

Well adding a has X11 to the ifdef would make that more clear. So how will this be done for EGL?

[Zeno Albisser]

Comment on attachment 167331 [details]
patch for review.

View in context: https://bugs.webkit.org/attachment.cgi?id=167331&action=review

>>>> Source/WebCore/platform/graphics/surfaces/GraphicsSurfaceToken.h:41
>>>> +    { }
>>> 
>>> So this covers linux without X11?
>> 
>> Linux without GLX/X11 will not use GRAPHICS_SURFACE in the first place. So it's not an issue.
> 
> Well adding a has X11 to the ifdef would make that more clear. So how will this be done for EGL?

Isn't that something we should worry about, once we are implementing a GraphicsSurface for EGL? I mean, it could be that a single uint32_t is perfectly viable for that case. Or it could be that we need two uint32_t to identify a front and a back buffer, as it is necessary for Mac. Or it could also be that we even need two uint64_t as we will need on Windows (ANGLE/EGL).
But I do not think it makes sense to try predicting all that before even prototyping such a solution for EGL. Let's take a step at the time.

[Noam Rosenthal]

Comment on attachment 167331 [details]
patch for review.

View in context: https://bugs.webkit.org/attachment.cgi?id=167331&action=review

>>>>> Source/WebCore/platform/graphics/surfaces/GraphicsSurfaceToken.h:41
>>>>> +    { }
>>>> 
>>>> So this covers linux without X11?
>>> 
>>> Linux without GLX/X11 will not use GRAPHICS_SURFACE in the first place. So it's not an issue.
>> 
>> Well adding a has X11 to the ifdef would make that more clear. So how will this be done for EGL?
> 
> Isn't that something we should worry about, once we are implementing a GraphicsSurface for EGL? I mean, it could be that a single uint32_t is perfectly viable for that case. Or it could be that we need two uint32_t to identify a front and a back buffer, as it is necessary for Mac. Or it could also be that we even need two uint64_t as we will need on Windows (ANGLE/EGL).
> But I do not think it makes sense to try predicting all that before even prototyping such a solution for EGL. Let's take a step at the time.

Still, HAVE(GLX) instead of OS(LINUX) makes this more readable and I fail to see the argument against that.

[Zeno Albisser]

Comment on attachment 167331 [details]
patch for review.

View in context: https://bugs.webkit.org/attachment.cgi?id=167331&action=review

>>>>>> Source/WebCore/platform/graphics/surfaces/GraphicsSurfaceToken.h:41
>>>>>> +    { }
>>>>> 
>>>>> So this covers linux without X11?
>>>> 
>>>> Linux without GLX/X11 will not use GRAPHICS_SURFACE in the first place. So it's not an issue.
>>> 
>>> Well adding a has X11 to the ifdef would make that more clear. So how will this be done for EGL?
>> 
>> Isn't that something we should worry about, once we are implementing a GraphicsSurface for EGL? I mean, it could be that a single uint32_t is perfectly viable for that case. Or it could be that we need two uint32_t to identify a front and a back buffer, as it is necessary for Mac. Or it could also be that we even need two uint64_t as we will need on Windows (ANGLE/EGL).
>> But I do not think it makes sense to try predicting all that before even prototyping such a solution for EGL. Let's take a step at the time.
> 
> Still, HAVE(GLX) instead of OS(LINUX) makes this more readable and I fail to see the argument against that.

How about context? - We distinguish between DARWIN and GLX? I think it is more readable to distinguish between DARWIN and LINUX.
Also i think it is not necessary to forcefully disable a code path that we do not even know yet, if it is going to be wrong for EGL.
But I am not going to argue too hard about it, it is just a single line after all. - So I will change it.

Anything else?

[Noam Rosenthal]

Comment on attachment 167331 [details]
patch for review.

View in context: https://bugs.webkit.org/attachment.cgi?id=167331&action=review

Some nitpicks... otherwise all good :)

> Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.cpp:37
> +void TextureMapperSurfaceBackingStore::setGraphicsSurface(GraphicsSurfaceToken graphicsSurfaceToken, const IntSize& surfaceSize, uint32_t frontBuffer)

const &?

> Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.h:53
> +    void setGraphicsSurface(GraphicsSurfaceToken, const IntSize& surfaceSize, uint32_t frontBuffer);

const &?

[Zeno Albisser]

Created attachment 167345 [details]
patch for review.

[Zeno Albisser]

Committed r131036: <http://trac.webkit.org/changeset/131036>