Bug 33852 – Implement HTMLVideoElement support for texImage2D and texSubImage2D

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Chris Marrin]

This will complete these calls.

[Adrienne Walker]

Created attachment 64358 [details]
Patch

[Chris Marrin]

Comment on attachment 64358 [details]
Patch


WebCore/html/canvas/WebGLRenderingContext.cpp:2213
 +      OwnPtr<ImageBuffer> buffer = ImageBuffer::create(size, DeviceRGB);
I don't think we should be creating and destroying the ImageBuffer on every call. It should be a member variable.

WebCore/html/canvas/WebGLRenderingContext.cpp:2477
 +      OwnPtr<ImageBuffer> buffer = ImageBuffer::create(size, DeviceRGB);

Same here


WebCore/html/canvas/WebGLRenderingContext.cpp:2485
 +                        m_unpackFlipY, m_unpackPremultiplyAlpha, ec);

No need to put this on a separate line

WebCore/html/canvas/WebGLRenderingContext.cpp:2221
 +                     m_unpackFlipY, m_unpackPremultiplyAlpha, ec);

No need to put this on a separate line

[Adrienne Walker]

(In reply to comment #2)
> (From update of attachment 64358 [details])
> 
> WebCore/html/canvas/WebGLRenderingContext.cpp:2213
>  +      OwnPtr<ImageBuffer> buffer = ImageBuffer::create(size, DeviceRGB);
> I don't think we should be creating and destroying the ImageBuffer on every call. It should be a member variable.

ImageBuffer objects can't be resized, but it is quite reasonable to keep one ImageBuffer around as a member variable and only recreate it when it needed to be resized.  The common case is probably that there's one video (or all the videos are identically sized).  We'd still need to recreate per-call in some cases, but hopefully not that often.

Is that what you mean, or am I misunderstanding?

> WebCore/html/canvas/WebGLRenderingContext.cpp:2221
>  +                     m_unpackFlipY, m_unpackPremultiplyAlpha, ec);
> 
> No need to put this on a separate line

Can do.  I was just making the code look like the rest of the file.

[Eric Carlson]

Comment on attachment 64358 [details]
Patch

> +    function startVideo() {
> +        // Resize canvas to fit video.
> +        var canvas = document.getElementsByTagName('canvas')[0];
> +        // If video has a broken height, pick an arbitrary canvas size.
> +        canvas.width = video.videoWidth > 0 ? video.videoWidth : 32;
> +        canvas.height = video.videoHeight > 0 ? video.videoHeight : 32;

If a video has videoWidth or videoHeight of 0 after the 'canplay' event fires, it either has no video track or there is a serious problem in the media engine. In either case, this test should fail instead of ignoring the problem.

> +    IntSize size(video->videoWidth(), video->videoHeight());
> +    OwnPtr<ImageBuffer> buffer = ImageBuffer::create(size, DeviceRGB);
> +    if (!buffer) {
> +        m_context->synthesizeGLError(GraphicsContext3D::OUT_OF_MEMORY);
> +        return;

If the video element is passed before metadata is available or if it has no visual media, videoWidth and videoHeight will be zero. ImageBuffer::create uses fastMalloc to allocate the backing store, which calls CRASH() if malloc() returns 0. Maybe you should do an early return with an INVALID_VALUE error instead?

> +    IntSize size(video->videoWidth(), video->videoHeight());
> +    OwnPtr<ImageBuffer> buffer = ImageBuffer::create(size, DeviceRGB);
> +    if (!buffer) {
> +        m_context->synthesizeGLError(GraphicsContext3D::OUT_OF_MEMORY);
> +        return;

Same here.

[Chris Marrin]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 64358 [details] [details])
> > 
> > WebCore/html/canvas/WebGLRenderingContext.cpp:2213
> >  +      OwnPtr<ImageBuffer> buffer = ImageBuffer::create(size, DeviceRGB);
> > I don't think we should be creating and destroying the ImageBuffer on every call. It should be a member variable.
> 
> ImageBuffer objects can't be resized, but it is quite reasonable to keep one ImageBuffer around as a member variable and only recreate it when it needed to be resized.  The common case is probably that there's one video (or all the videos are identically sized).  We'd still need to recreate per-call in some cases, but hopefully not that often.
> 
> Is that what you mean, or am I misunderstanding?

That's what I mean. The problem is that this gets called a lot for video (ideally at 30fps) and this allocation will be expensive and will fragment memory. But I see a problem caching it in the GraphicsContext3D. You will get a lot of churn if you, for instance, try to render two videos of different sizes. You will continually recreate the ImageBuffer.

The right solution might be to cache several ImageBuffers using the size as the key. As long as you only cache a few buffers it should not be too expensive. Another option would be to cache the ImageBuffer in the HTMLVideoElement.

Eric, what do you think is the right thing to do here?

[Eric Carlson]

(In reply to comment #5)
> The right solution might be to cache several ImageBuffers using the size as the key. As long as 
> you only cache a few buffers it should not be too expensive. Another option would be to cache 
> the ImageBuffer in the HTMLVideoElement.
> 
> Eric, what do you think is the right thing to do here?

I would guess that *most* of the time a given canvas element will only have one movie drawn to it at a time, but if it is important to optimize for multiple movies I think keeping a small number of ImageBuffers would be the way to go.

[Adrienne Walker]

Created attachment 64748 [details]
Patch

[Adrienne Walker]

(In reply to comment #7)
> Created an attachment (id=64748) [details]
> Patch

I wanted to comment and say that I added an array-based LRU image buffer cache of size 4.  Because the consensus seemed to be that we should just keep a small number of image buffer sizes around, this array approach seemed reasonable for small sizes and much less complex than a full hash table plus doubly linked list implementation.

[WebKit Review Bot]

Attachment 64748 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3761336

[Chris Marrin]

Comment on attachment 64748 [details]
Patch

WebCore/html/canvas/WebGLRenderingContext.cpp:2484
 +      texSubImage2DImpl(target, level, xoffset, yoffset, format, type, buf->image(), m_unpackFlipY, m_unpackPremultiplyAlpha, ec);


You need to update your patch to take into account Dave Hyatt's recent change to ImageBuffer where he gets rid of ImageBuffer::image. There is a new way to do this now, but I'm not sure what it is.

[Chris Marrin]

Comment on attachment 64748 [details]
Patch

WebCore/html/canvas/WebGLRenderingContext.cpp:3803
 +  ImageBuffer* WebGLRenderingContext::LRUImageBufferCache::Get(const IntSize& size)


The convention is to start functions with lower case. Better than using get(), imageBuffer() would be a better name, since that's what you're returning



WebCore/html/canvas/WebGLRenderingContext.cpp:3812
 +          BubbleToFront(i);


Start function with lower case

[Chris Marrin]

Comment on attachment 64748 [details]
Patch

WebCore/html/canvas/WebGLRenderingContext.cpp:3803
 +  ImageBuffer* WebGLRenderingContext::LRUImageBufferCache::Get(const IntSize& size)


The convention is to start functions with lower case. Better than using get(), imageBuffer() would be a better name, since that's what you're returning



WebCore/html/canvas/WebGLRenderingContext.cpp:3812
 +          BubbleToFront(i);


Start function with lower case



WebCore/html/canvas/WebGLRenderingContext.cpp:3827
 +  void WebGLRenderingContext::LRUImageBufferCache::BubbleToFront(int idx)


Here too




WebCore/html/canvas/WebGLRenderingContext.h:427
 +      protected:


Why protected rather than private? Will this ever be subclassed?

[Chris Marrin]

(In reply to comment #11)
> (From update of attachment 64748 [details])
> WebCore/html/canvas/WebGLRenderingContext.cpp:3803
>  +  ImageBuffer* WebGLRenderingContext::LRUImageBufferCache::Get(const IntSize& size)
> 
> 
> The convention is to start functions with lower case. Better than using get(), imageBuffer() would be a better name, since that's what you're returning
> 
> 
> 
> WebCore/html/canvas/WebGLRenderingContext.cpp:3812
>  +          BubbleToFront(i);
> 
> 
> Start function with lower case

Please ignore these comments, they are duplicates in the following comments

[Adrienne Walker]

Created attachment 64783 [details]
Patch

[Kenneth Russell]

Comment on attachment 64783 [details]
Patch

This looks very good overall. A couple of comments.

LayoutTests/fast/canvas/webgl/tex-image-and-sub-image-2d-with-video.html:64
 +          1.0, 0.0]);
Please look at the current version of tex-image-and-sub-image-2d-with-image.html for a starting point for this test. It uses WebGLTestUtils and in particular setupTexturedQuad which should make this code much more terse.


WebCore/html/canvas/WebGLRenderingContext.cpp:2209
 +      RefPtr<Image> image = buf->copyImage();
Is there a possibility of sharing this block of code between the texImage2D and texSubImage2D variants? Also, I think it's worth adding a FIXME about the fact that we're using copyImage here. At some point this copy is going to be a bottleneck for applications and we'll want to revisit it. On most platforms we'll eventually want to turn this into a GPU-to-GPU texture copy when the infrastructure is available.

[Adrienne Walker]

Created attachment 64872 [details]
Patch

[Kenneth Russell]

Comment on attachment 64872 [details]
Patch

Very nice. r=me. Let me know if you need help getting this patch checked in.

[Chris Marrin]

(In reply to comment #15)
> (From update of attachment 64783 [details])
> This looks very good overall. A couple of comments.
> 
> LayoutTests/fast/canvas/webgl/tex-image-and-sub-image-2d-with-video.html:64
>  +          1.0, 0.0]);
> Please look at the current version of tex-image-and-sub-image-2d-with-image.html for a starting point for this test. It uses WebGLTestUtils and in particular setupTexturedQuad which should make this code much more terse.
> 
> 
> WebCore/html/canvas/WebGLRenderingContext.cpp:2209
>  +      RefPtr<Image> image = buf->copyImage();
> Is there a possibility of sharing this block of code between the texImage2D and texSubImage2D variants? Also, I think it's worth adding a FIXME about the fact that we're using copyImage here. At some point this copy is going to be a bottleneck for applications and we'll want to revisit it. On most platforms we'll eventually want to turn this into a GPU-to-GPU texture copy when the infrastructure is available.

Note that I believe ImageBuffer was ALWAYS doing a copyImage before. It's just that now you only doing when you ask for it.

But I agree that we eventually need a way to get video into the GPU more efficiently. This is especially bad since often the video is alway in backing store on the GPU for use by the HTML compositor. So we are currently pulling it out of the GPU and then pushing it right back in.

[Simon Fraser (smfr)]

(In reply to comment #18)

> But I agree that we eventually need a way to get video into the GPU more efficiently. This is especially bad since often the video is alway in backing store on the GPU for use by the HTML compositor. So we are currently pulling it out of the GPU and then pushing it right back in.

This deserves a new bug.

[Adrienne Walker]

(In reply to comment #18)
> Note that I believe ImageBuffer was ALWAYS doing a copyImage before. It's just that now you only doing when you ask for it.

You're correct.
 
> But I agree that we eventually need a way to get video into the GPU more efficiently. This is especially bad since often the video is alway in backing store on the GPU for use by the HTML compositor. So we are currently pulling it out of the GPU and then pushing it right back in.

My thought was that an inefficient, but simple and cross-platform approach would make a reasonable first cut for this feature.  It works well enough to smoothly play a 400x300 video at 30fps in a WebGL texture on my machine.

I absolutely agree that texImage2D calls with image, video, and canvas elements should eventually get optimized to avoid the needless readback to the CPU from the GPU.

[WebKit Commit Bot]

Comment on attachment 64872 [details]
Patch

Rejecting patch 64872 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Kenneth Russell', u'--force']" exit_code: 255
Parsed 8 diffs from patch file(s).
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
literal is only avaliable with the XS version at /Library/Perl/5.8.8/Compress/Zlib.pm line 9
BEGIN failed--compilation aborted at /Library/Perl/5.8.8/Compress/Zlib.pm line 9.
Compilation failed in require at /Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/VCSUtils.pm line 1662.

Full output: http://queues.webkit.org/results/3710423

[Adam Barth]

GIT binary patch
literal 1578

Sorry svn-apply can't handle these sorts of patches.  :(

[Adrienne Walker]

(In reply to comment #22)
> GIT binary patch
> literal 1578
> 
> Sorry svn-apply can't handle these sorts of patches.  :(

Oops. I didn't realize that webkit-patch would fail me like that.  I see the bug is already filed, though.

[Adam Barth]

> Oops. I didn't realize that webkit-patch would fail me like that.  I see the bug is already filed, though.

Yeah, it's one of our top trouble spots.  Hopefully one of our illustrious Perl hackers will make svn-apply more awesome.

[Eric Seidel (no email)]

svn-apply can handle "literal" diffs, just not "delta" diffs.

[Eric Seidel (no email)]

The problem here is that the commit-queue has some broken perl install.  I need to fix it.  For the moment, we'll have to land this by hand.  I plan to move the cq to Snow Leopard next week. :(

[Adrienne Walker]

(In reply to comment #19)

> This deserves a new bug.

Agreed.  Filed here: https://bugs.webkit.org/show_bug.cgi?id=44339

[Kenneth Russell]

Committed r65756: <http://trac.webkit.org/changeset/65756>

[WebKit Review Bot]

http://trac.webkit.org/changeset/65756 might have broken Chromium Mac Release