Using glMapTexSubImage2d in VideoLayerChromium

• Component: New Bugs
• Status: RESOLVED FIXED

[Victoria Kirst]

Using glMapTexSubImage2d in VideoLayerChromium

[Victoria Kirst]

Created attachment 62789 [details]
Patch

[Andrew Scherkus]

Comment on attachment 62789 [details]
Patch

Few tiny nits... we'll probably need vangelis/darin to take a look.


WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:99
 +      // This is needed to get text to show up correctly. Without it,
indentation

WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:119
 +  #error "Need to implement for your platform."
Do you know if this will this break Chromium Mac since they don't use Skia?

WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:129
 +      ASSERT(skiaBitmap);
I think this will always be true since a reftype (const SkBitmap&) can't be NULL (it's sitting on your stack or in heap memory somewhere).  Therefore taking an address of that will always produce a non-NULL address.  Actually do we even need skiaBitmap?  It looks like you can do all operations in this function using bitmap.

WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:158
 +      ASSERT(skiaBitmap);
ditto for here

WebCore/platform/graphics/chromium/VideoLayerChromium.h:59
 +  
remove extra lines

[Victoria Kirst]

(In reply to comment #2)
> (From update of attachment 62789 [details])
> Few tiny nits... we'll probably need vangelis/darin to take a look.
> 
> 
> WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:99
>  +      // This is needed to get text to show up correctly. Without it,
> indentation
Thanks! Fixed.


> WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:119
>  +  #error "Need to implement for your platform."
> Do you know if this will this break Chromium Mac since they don't use Skia?
Not sure, but it looks to be the case. Not sure how we handle Mac stuff. 

BTW, what I did to implement this version of VideoLayerChromium.cpp is I copied everything in LayerChromium.cpp then changed the code as necessary to make it work for glMapTexSubImage2D, so problems here are also in LayerChromium. But this brings up a related point, there's nothing really video-specific to what I'm doing here in VideoLayerChromium yet. Do you think I should actually be modifying LayerChromium.cpp with these changes, then have VideoLayerChromium.cpp essentially still just inherit LayerChromium.cpp behavior?

> WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:129
>  +      ASSERT(skiaBitmap);
> I think this will always be true since a reftype (const SkBitmap&) can't be NULL (it's sitting on your stack or in heap memory somewhere).  Therefore taking an address of that will always produce a non-NULL address.  Actually do we even need skiaBitmap?  It looks like you can do all operations in this function using bitmap.

Right, again I was copying what was done in LayerChromium.cpp, so I thought it might be some WebKit stylistic thing to make the code arguably more readable by avoiding reftypes. Let me know what you think is best, and I should probably update LayerChromium.cpp to match (or like I said earlier, maybe I should be modifying LayerChromium.cpp to begin with).

> 
> WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:158
>  +      ASSERT(skiaBitmap);
> ditto for here
> 
> WebCore/platform/graphics/chromium/VideoLayerChromium.h:59
>  +  
> remove extra lines
Thanks! Done.

[Victoria Kirst]

Created attachment 62849 [details]
Patch

[Vangelis Kokkevis]

I think that if you're relying on the GraphicsContext to render the contents of your video layer then there's no reason to create a different layer type for it. I thought that the video layer would have direct pointer
to the memory containing the frame data so that it can populate the texture directly from there. With the code the way it is, every time you get a new video frame you will create a temporary GraphicsContext and Skia canvas of that
size (one large mem allocation), call the GraphicsContext to render the video frame to it (one large mem copy) and then update the texture.

The code for using mapped memory for texture updates should move over to LayerChromium as you suggested.

[Andrew Scherkus]

I guess right now we're not doing anything too special but the next step is doing YUV + shader stuff where we will have access to the frame data directly.

[Vangelis Kokkevis]

(In reply to comment #6)
> I guess right now we're not doing anything too special but the next step is doing YUV + shader stuff where we will have access to the frame data directly.

Once you have access to the frame data, most of this code will go away. Would it make sense to wire that direct access up (even if you get an RGB buffer) instead of going through this intermediate step?

[Victoria Kirst]

(In reply to comment #5)
> I think that if you're relying on the GraphicsContext to render the contents of your video layer then there's no reason to create a different layer type for it. 

Actually, this implementation is still somewhat just a rough, crappy implementation of VideoLayerChromium. Sorry I didn't make that clear! In our hypothetical final version, we would not use GraphicsContext to render the contents of the video layer.

> I thought that the video layer would have direct pointer to the memory containing the frame data so that it can populate the texture directly from there. 

Yes, this is what we want to do! Our next steps are figuring out how we can best get the direct pointer of frame data to the VideoLayerChromium, but in the meantime we wanted some crappy VideoLayerChromium to play around with (which is this, haha). Happily, even this crappy version gives us a slight (5-10%) CPU usage decrease.

> The code for using mapped memory for texture updates should move over to LayerChromium as you suggested.

This is actually turning out to be a bigger change than I thought since it's going to change some of the LayerChromium API, so it's something that we should probably talk about before I just hack away. The issue in particular is that updateTextureRect() is a protected function when I thought it was a private function. updateTextureRect() takes a void* pixels, which means that in order to use glMapTexSubImage2D, you are forced to do a memcpy from pixels to the texture memory mapped out from glMapTexSubImage2D. I think this would be a superfluous memcpy in many cases, including in VideoLayerChromium. 

Anyway, I would like to just keep this VideoLayerChromium for now, knowing that it's going to change a lot when we figure out how to get the frame data to it. LayerChromium needs to be revised to use glMapTexSubImage2D -- and I'd be happy to do it -- but I think we need to discuss it a bit more and it should certainly be in a different cl.

[Hin-Chung Lam]

I think this is an important step as proof of concept and showed how to use the gl commands properly.

The final version will have a different approach for getting the video content, but the commands for uploading texture content will still be the same (just more uploads). So I think this is a nice stop for us to iterate.

This patch is looking good to me, is there any other comments?

[Vangelis Kokkevis]

(In reply to comment #9)
> I think this is an important step as proof of concept and showed how to use the gl commands properly.
> 
> The final version will have a different approach for getting the video content, but the commands for uploading texture content will still be the same (just more uploads). So I think this is a nice stop for us to iterate.
> 
> This patch is looking good to me, is there any other comments?

No objections here to checking it in although I really do think that almost all the code that's in the VideoLayerChromium will disappear once you stop rendering via the GraphicsContext.

[David Levin]

Comment on attachment 62849 [details]
Patch

Just a few very trivial details that it would be nice to clean up.

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> [phone]  Victoria Kirst  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Added logic to use glMapTexSubImage2D to write video layer to GPU
> +        texture. Also fixes CPU usage problem from previous patch.
> +        https://bugs.webkit.org/show_bug.cgi?id=43101

Please explain why there are no tests added.

"No change in user visible functionality (since it isn't turned on), so no new tests."

When this does become enabled either there needs to be tests or an indication of what tests cover the functionality.


> diff --git a/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp b/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp
>  
> +#include "LayerRendererChromium.h"
> +#if PLATFORM(SKIA)
> +#include "NativeImageSkia.h"
> +#include "PlatformContextSkia.h"
> +#endif

#if'ed includes should come after all other includes.

> +#include "RenderLayerBacking.h"
> +#include "skia/ext/platform_canvas.h"
> +
> +#include <GLES2/gl2.h>
> +
> +#define GL_GLEXT_PROTOTYPES
> +#include <GLES2/gl2ext.h>
> +

> +    // If the texture needs to be reallocated then we must redraw the entire

Please add a "," before then.


> +        // FIXME: Does this take us down a very slow text rendering path?
> +        // FIXME: why is this is a windows-only call ?

*W*hy (and the space before the ? is odd.)


> +    // If the texture id or size changed since last time then we need to tell GL
Please add a "," before then.

[Victoria Kirst]

Created attachment 63354 [details]
Patch

[Victoria Kirst]

Comment on attachment 63354 [details]
Patch

Fixed code based on Dave's suggestions. Thanks!

[WebKit Commit Bot]

Comment on attachment 63354 [details]
Patch

Clearing flags on attachment: 63354

Committed r64731: <http://trac.webkit.org/changeset/64731>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.