Bug 33852

Summary: Implement HTMLVideoElement support for texImage2D and texSubImage2D
Product: WebKit Reporter: Chris Marrin <cmarrin>
Component: WebGLAssignee: Adrienne Walker <enne>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, commit-queue, dbates, dglazkov, enne, eric.carlson, eric, kbr, simon.fraser, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Macintosh   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch kbr: review+, commit-queue: commit-queue-

Description Chris Marrin 2010-01-19 11:34:46 PST
This will complete these calls.
Comment 1 Adrienne Walker 2010-08-13 11:24:11 PDT
Created attachment 64358 [details]
Patch
Comment 2 Chris Marrin 2010-08-13 12:20:59 PDT
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
Comment 3 Adrienne Walker 2010-08-13 12:31:37 PDT
(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.
Comment 4 Eric Carlson 2010-08-13 13:54:02 PDT
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.
Comment 5 Chris Marrin 2010-08-13 14:43:28 PDT
(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?
Comment 6 Eric Carlson 2010-08-13 15:20:30 PDT
(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.
Comment 7 Adrienne Walker 2010-08-18 12:06:59 PDT
Created attachment 64748 [details]
Patch
Comment 8 Adrienne Walker 2010-08-18 12:11:07 PDT
(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.
Comment 9 WebKit Review Bot 2010-08-18 13:11:29 PDT
Attachment 64748 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3761336
Comment 10 Chris Marrin 2010-08-18 13:35:28 PDT
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.
Comment 11 Chris Marrin 2010-08-18 13:39:44 PDT
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
Comment 12 Chris Marrin 2010-08-18 13:44:15 PDT
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?
Comment 13 Chris Marrin 2010-08-18 13:45:03 PDT
(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
Comment 14 Adrienne Walker 2010-08-18 15:47:55 PDT
Created attachment 64783 [details]
Patch
Comment 15 Kenneth Russell 2010-08-18 18:25:53 PDT
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.
Comment 16 Adrienne Walker 2010-08-19 10:52:21 PDT
Created attachment 64872 [details]
Patch
Comment 17 Kenneth Russell 2010-08-19 11:53:11 PDT
Comment on attachment 64872 [details]
Patch

Very nice. r=me. Let me know if you need help getting this patch checked in.
Comment 18 Chris Marrin 2010-08-19 15:02:14 PDT
(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.
Comment 19 Simon Fraser (smfr) 2010-08-19 15:07:40 PDT
(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.
Comment 20 Adrienne Walker 2010-08-19 15:30:59 PDT
(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.
Comment 21 WebKit Commit Bot 2010-08-19 15:54:10 PDT
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
Comment 22 Adam Barth 2010-08-19 15:57:13 PDT
GIT binary patch
literal 1578

Sorry svn-apply can't handle these sorts of patches.  :(
Comment 23 Adrienne Walker 2010-08-19 16:16:01 PDT
(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.
Comment 24 Adam Barth 2010-08-19 16:18:50 PDT
> 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.
Comment 25 Eric Seidel (no email) 2010-08-19 16:24:42 PDT
svn-apply can handle "literal" diffs, just not "delta" diffs.
Comment 26 Eric Seidel (no email) 2010-08-19 16:25:24 PDT
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. :(
Comment 27 Adrienne Walker 2010-08-20 09:43:09 PDT
(In reply to comment #19)

> This deserves a new bug.

Agreed.  Filed here: https://bugs.webkit.org/show_bug.cgi?id=44339
Comment 28 Kenneth Russell 2010-08-20 15:12:22 PDT
Committed r65756: <http://trac.webkit.org/changeset/65756>
Comment 29 WebKit Review Bot 2010-08-20 15:45:49 PDT
http://trac.webkit.org/changeset/65756 might have broken Chromium Mac Release