Bug 77923 - [chromium] Allow retaining texture across frames in composited video playback and correctly handle lost context
Summary: [chromium] Allow retaining texture across frames in composited video playback...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: James Robinson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-06 18:36 PST by James Robinson
Modified: 2012-02-07 17:28 PST (History)
4 users (show)

See Also:


Attachments
Patch (3.68 KB, patch)
2012-02-06 18:38 PST, James Robinson
kbr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2012-02-06 18:36:20 PST
[chromium] Allow retaining texture across frames in composited video playback and correctly handle lost context
Comment 1 James Robinson 2012-02-06 18:38:07 PST
Created attachment 125748 [details]
Patch
Comment 2 James Robinson 2012-02-06 18:40:06 PST
This improves the composited video playback a good bit. It depends on http://trac.webkit.org/changeset/106840, in case we want to merge this.

Ami - we avoid regressing lost context case for other types of content by adding layout tests like LayoutTests/platform/chromium/compositing/lost-compositor-context*. I'd like to do the same for html5 video but am not sure how to make a reliable layout test involving video playback, do you have any hints or suggestions or pointers to similar tests that I could copy? I'd like to make sure we draw at least one frame, lost the compositor context, then draw at least one more frame from the video.
Comment 3 James Robinson 2012-02-06 18:41:19 PST
Actually, depends on http://trac.webkit.org/changeset/106891 as well since I'm using ManagedTexture::setTextureManager()
Comment 4 Ami Fischman 2012-02-06 19:00:04 PST
(In reply to comment #2)
> This improves the composited video playback a good bit.

OOC, how do you observe this?

> Ami - we avoid regressing lost context case for other types of content by adding layout tests like LayoutTests/platform/chromium/compositing/lost-compositor-context*.

Oooh, nice.

> I'd like to do the same for html5 video but am not sure how to make a reliable layout test involving video playback, do you have any hints or suggestions or pointers to similar tests that I could copy? I'd like to make sure we draw at least one frame, lost the compositor context, then draw at least one more frame from the video.

http://trac.webkit.org/browser/trunk/LayoutTests/media/ is where most video-related layouttests live.  
I think you could have a reliable test by seeking a video that hasn't been play()'d yet to a known spot, losing the context, and seeking to another spot, asserting the expected currentTime when the latter 'seeked' event fires.
Comment 5 Ami Fischman 2012-02-06 19:00:39 PST
Also, the patch lgtm, fwiw.  Thanks!
Comment 6 Vangelis Kokkevis 2012-02-06 22:18:46 PST
Comment on attachment 125748 [details]
Patch

Looks great!
Comment 7 Kenneth Russell 2012-02-07 14:18:25 PST
Comment on attachment 125748 [details]
Patch

rs=me based on Vangelis's review.
Comment 8 James Robinson 2012-02-07 14:20:45 PST
Committed r106992: <http://trac.webkit.org/changeset/106992>
Comment 9 James Robinson 2012-02-07 17:28:26 PST
(In reply to comment #4)
> (In reply to comment #2)
> > This improves the composited video playback a good bit.
> 
> OOC, how do you observe this?

I should clarify - it improves the behavior I see in tracing (there aren't any new textures created) but I haven't measured any framerate improvement or anything like that.

> 
> > Ami - we avoid regressing lost context case for other types of content by adding layout tests like LayoutTests/platform/chromium/compositing/lost-compositor-context*.
> 
> Oooh, nice.
> 
> > I'd like to do the same for html5 video but am not sure how to make a reliable layout test involving video playback, do you have any hints or suggestions or pointers to similar tests that I could copy? I'd like to make sure we draw at least one frame, lost the compositor context, then draw at least one more frame from the video.
> 
> http://trac.webkit.org/browser/trunk/LayoutTests/media/ is where most video-related layouttests live.  
> I think you could have a reliable test by seeking a video that hasn't been play()'d yet to a known spot, losing the context, and seeking to another spot, asserting the expected currentTime when the latter 'seeked' event fires.

Added a layout test here: https://bugs.webkit.org/show_bug.cgi?id=78060 that I believe would have caught this regression