Bug 73043 – Teach VideoLayerChromium how to render native texture (to support HW video decode).

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ami Fischman]

Teach VideoLayerChromium how to render native texture (to support HW video decode).

[Ami Fischman]

Created attachment 116406 [details]
Patch

[Ami Fischman]

Ken: vrk@ suggested you'd be a good person to review this.  If that's wrong do you know who would be?

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[WebKit Review Bot]

Comment on attachment 116406 [details]
Patch

Attachment 116406 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10630075

[Ami Fischman]

WebKit-n00b time: chromium-ews bot failed b/c I added a pure-virtual to WebKit::WebVideoFrame, implemented in chromium's webkit_media::WebVideoFrameImpl (and of course the chromium change isn't included in this patch, so the bot doesn't see the method implementation).

In order to land this do I need to first land the chromium-side patch to add the implementation (sans OVERRIDE), then wait for webkit to do a chromium roll, then add the pure virtual to webkit, then wait for chromium to do a webkit roll, then add OVERRIDE to chromium?

[Kenneth Russell]

(In reply to comment #5)
> WebKit-n00b time: chromium-ews bot failed b/c I added a pure-virtual to WebKit::WebVideoFrame, implemented in chromium's webkit_media::WebVideoFrameImpl (and of course the chromium change isn't included in this patch, so the bot doesn't see the method implementation).
> 
> In order to land this do I need to first land the chromium-side patch to add the implementation (sans OVERRIDE), then wait for webkit to do a chromium roll, then add the pure virtual to webkit, then wait for chromium to do a webkit roll, then add OVERRIDE to chromium?

To simplify the landing process, you can provide an empty implementation of the pure virtual for the time being. It would be best to add a FIXME about making it pure virtual, and file and take a follow-on bug.

[Ami Fischman]

Created attachment 116414 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 116414 [details]
Patch

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

> Source/WebKit/chromium/public/WebVideoFrame.h:63
> +    virtual unsigned textureId() const { return 0; } // FIXME: make pure virtual once chromium-side lands (bug 73048).

Actually, methods intended to be implemented by Chromium are normally not made pure virtual.  Normally, we provide default implementations here as you are doing for this method.

[Ami Fischman]

(In reply to comment #8)
> (From update of attachment 116414 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=116414&action=review
> 
> > Source/WebKit/chromium/public/WebVideoFrame.h:63
> > +    virtual unsigned textureId() const { return 0; } // FIXME: make pure virtual once chromium-side lands (bug 73048).
> 
> Actually, methods intended to be implemented by Chromium are normally not made pure virtual.  Normally, we provide default implementations here as you are doing for this method.

Huh.  There're an awful lot of "= 0;"'s in this class and in this directory, though.
Happy to go either way, of course.

[Darin Fisher (:fishd, Google)]

(In reply to comment #9)
> Huh.  There're an awful lot of "= 0;"'s in this class and in this directory, though.
> Happy to go either way, of course.

We're not consistent.  Keep in mind that interfaces implemented by WebKit should indeed have pure virtual methods.

The point of saying that embedder implemented interfaces should not use pure virtual methods is to avoid the churn you are experiencing with this patch today.  You are putting in a FIXME comment, which implies having to go back and cleanup the code later to remove the FIXME.  If you could avoid that round of busy work, wouldn't you appreciate it? :-)

[Ami Fischman]

(In reply to comment #10)
> (In reply to comment #9)
> > Huh.  There're an awful lot of "= 0;"'s in this class and in this directory, though.
> > Happy to go either way, of course.
> 
> We're not consistent.  Keep in mind that interfaces implemented by WebKit should indeed have pure virtual methods.
> 
> The point of saying that embedder implemented interfaces should not use pure virtual methods is to avoid the churn you are experiencing with this patch today.  You are putting in a FIXME comment, which implies having to go back and cleanup the code later to remove the FIXME.  If you could avoid that round of busy work, wouldn't you appreciate it? :-)

Yes, I would :)
I am not clear on what you are telling me to do, though.  Options I see include:
1) Remove the FIXME, leaving the impl in place, leaving the rest of the class as-is.
2) Replace all the =0's in the class with empty impl's and remove the FIXME.

Did you mean one of these or something else?

[Kenneth Russell]

(In reply to comment #10)
> (In reply to comment #9)
> > Huh.  There're an awful lot of "= 0;"'s in this class and in this directory, though.
> > Happy to go either way, of course.
> 
> We're not consistent.  Keep in mind that interfaces implemented by WebKit should indeed have pure virtual methods.
> 
> The point of saying that embedder implemented interfaces should not use pure virtual methods is to avoid the churn you are experiencing with this patch today.  You are putting in a FIXME comment, which implies having to go back and cleanup the code later to remove the FIXME.  If you could avoid that round of busy work, wouldn't you appreciate it? :-)

This is good to know. I'd been assuming that all interfaces in the WebKit API (both directions) were supposed to be pure virtual. Ami added the FIXME and the follow-on bug on my request; Ami, please feel free to remove the FIXME and close the other bug on Darin's feedback.

[Ami Fischman]

Created attachment 116432 [details]
Patch

[Ami Fischman]

Went with option 2 - replace all =0's in the class w/ empty impls.

[Darin Fisher (:fishd, Google)]

Comment on attachment 116432 [details]
Patch

WebKit API changes LGTM

[Kenneth Russell]

Comment on attachment 116432 [details]
Patch

Looks good to me. Style failure looks like a flake. r=me

[WebKit Review Bot]

Comment on attachment 116432 [details]
Patch

Clearing flags on attachment: 116432

Committed r101115: <http://trac.webkit.org/changeset/101115>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[James Robinson]

I'm disappointed this was landed without tests.  New logic without tests is likely to break quickly.

I also think it's a mistake to bake this new drawing path into the existing VideoLayer stuff. It's a different enough rendering path - uses different resources, shaders, and ownership - that it deserves its own layer type.

What is this I420 format stuff? I don't see it being used.

Reopening for test coverage.  We cannot support untested codepaths in the compositor.

[Ami Fischman]

(In reply to comment #19)
> I'm disappointed this was landed without tests.  New logic without tests is likely to break quickly.

Teach me to fish.  How does a change to the chromium compositor in WK get tested before the chromium-side implementation lands (and rolls into WK)?  I'd love for that to exist independent of the chromium implementation.

> I also think it's a mistake to bake this new drawing path into the existing VideoLayer stuff. It's a different enough rendering path - uses different resources, shaders, and ownership - that it deserves its own layer type.

I started out hoping to reuse the Plugin* layer/renderer for this (and rename it s/Plugin/External/g or somesuch) but was confounded by the fact that the layer/renderer needs to be decided on & created when the element is created (WebMediaPlayerClientImpl in this case), which is too early to know whether software or hardware decode will be used (a decision that can only be reached after the media stream starts loading and demuxing).

I am not at all wedded to anything in this implementation (other than it working :)).  Happy to make changes if you have alternate suggestions.

> What is this I420 format stuff? I don't see it being used.

Assuming you mean the change to WebVideoFrame.h, I made it to match chromium's media::VideoFrame, which WebVideoFrame is the counterpart to.  Apparently I420 wasn't added to the WK side when it was added to the chromium and I made them symmetric again, since chromium expects the enums to match up.
media::VideoFrame: http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/media/base/video_frame.h&exact_package=chromium&q=file:media/base/video_frame.h%20I420&type=cs&l=39
Expectation of matching up: http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/webkit/media/webvideoframe_impl.cc&exact_package=chromium&q=COMPILE_ASSERT%20VideoFrame%20WebVideoFrame&type=cs&l=35

> Reopening for test coverage.  We cannot support untested codepaths in the compositor.

Thanks for reopening - it was not my intent to declare Mission Accomplished before some test coverage was achieved (which I assumed would require the chromium-side changes to land first).

[James Robinson]

(In reply to comment #20)
> (In reply to comment #19)
> > I'm disappointed this was landed without tests.  New logic without tests is likely to break quickly.
> 
> Teach me to fish.  How does a change to the chromium compositor in WK get tested before the chromium-side implementation lands (and rolls into WK)?  I'd love for that to exist independent of the chromium implementation.
> 

Sorry for complaining and then running, I've been a bit overloaded this week (and every other week it seems).

The things about this patch that I'd most like to see test coverage for are:

1.) Frame lifetime - making sure we don't use try to draw from textures when we should not, making sure we pick up new frames when we should, and making sure we do the correct thing when the video frame format changes. I have some questions about this part of the patch as it exists, actually.

2.) Drawing frames - making sure that drawNativeTexture() actually puts the video frame we want on screen and isn't upside down or color inverted or unscaled. That function sets some geometry and texture units up, but that logic isn't tested.  It looks like a lot of this logic is identical to the logic we use for drawing other types - like RGBA video texture format, plugin layers, and single-tile content layers, and images. I would like to be able to unify the draw logic for these and have confidence that I'm not breaking video display.

As for (1) I'm not quite sure how it's supposed to work today. It seems that once a texture ID is grabbed from the VideoFrameProvider it is valid to bind and draw from, but for how long? Is it valid to draw from the texture an arbitrary number of times between frames? When the video is torn down or the decoding pipeline is torn down, at what point does it become no longer valid to draw from the native texture and how is that information communicated to the VideoLayer?  I think there are some hidden assumptions being made here, but without knowing what they are it's hard to know exactly how to test for them.

When a new frame becomes available, do we need to do anything other than re-binding the native texture ID to ensure the frame is available? Do we get a different native texture ID for each frame? Does the decoder take care of ensuring that we can never see an intermediate result, no matter when we draw?

As for (2) there's an example of a layout test to check the pixel output from a texture managed externally to the compositor here: https://bugs.webkit.org/show_bug.cgi?id=69027.  That test is set up for plugin layers.  The draw path for plugin layers seems pretty much identical to that for native textures.  In fact, if it used the same compositor drawing code then the plugin test could provide test coverage for videos too.  Since they are currently using different code to set up the draw transforms, attributes, etc we don't get that benefit.

[James Robinson]

> I started out hoping to reuse the Plugin* layer/renderer for this (and rename it s/Plugin/External/g or somesuch) but was confounded by the fact that the layer/renderer needs to be decided on & created when the element is created (WebMediaPlayerClientImpl in this case), which is too early to know whether software or hardware decode will be used (a decision that can only be reached after the media stream starts loading and demuxing).
> 
> I am not at all wedded to anything in this implementation (other than it working :)).  Happy to make changes if you have alternate suggestions.
> 

I see, it's not quite as simple as I thought.  I'd be happy to try some ways to simplify this if I had a way to tell if I was breaking the video path or not while doing so.

[Ami Fischman]

[Sorry for the delay, also :)  (been sick/recovering)]

- Lifecycle: as long as the compositor can get a frame (as "current"), the chromium-side code keeps the backing texture in a state where it can be read from (and doesn't overwrite it with new data until it stops being gotten by the compositor).  I am not at all confident in this part of the CL.  Are you aware of any document/comments/anything that describes the intended lifecycle of LayerChromium's updateCompositorResources()/pushPropertiesTo() and CCLayerImpl's draw()?  (my impl cargo-culted what I found in  VideoLayer and PluginLayer).

- Drawing code: indeed, drawNativeTexture & drawRGBA are virtually identical.  My first stab at it actually extracted it to a helper but that ended up having to be templated (to support the different *Program types) and looked sufficiently complicated that I decided to drop it.  Are you aware of any tests for the drawRGBA stuff (other than layout tests, which require the chromium side land, obvs)?

> As for (1) I'm not quite sure how it's supposed to work today. It seems that once a texture ID is grabbed from the VideoFrameProvider it is valid to bind and draw from, but for how long?

The provider tracks which frame is "current" and keeps it valid.
(chromium CL is http://codereview.chromium.org/8686010/ in case that helps)

> Is it valid to draw from the texture an arbitrary number of times between frames? 

Yeah.

> When the video is torn down or the decoding pipeline is torn down, at what point does it become no longer valid to draw from the native texture and how is that information communicated to the VideoLayer?  I think there are some hidden assumptions being made here, but without knowing what they are it's hard to know exactly how to test for them.

Agreed (and that the assumptions are hidden from me, as well).  Hopefully answers to my questions above will enable me to answer this.

> When a new frame becomes available, do we need to do anything other than re-binding the native texture ID to ensure the frame is available? 

No.

> Do we get a different native texture ID for each frame?

Maybe maybe not.  The provider reuses textures, so IDs will be re-seen over time.
(my impl uses 4 textures).

> Does the decoder take care of ensuring that we can never see an intermediate result, no matter when we draw?

Yes; frames are only handed to compositor once ready, and aren't re-used by the decoder until they're no longer current.

> As for (2) there's an example of a layout test to check the pixel output from a texture managed externally to the compositor here: https://bugs.webkit.org/show_bug.cgi?id=69027.

Ah; I was thinking using a layouttest for testing this would be blocked on the chromium impl, but I think you're saying that that's not the case, since the chromium/DumpRenderTree (in WK) can implement the client-side.  I'll look at doing this tomorrow.

[James Robinson]

(In reply to comment #23)
> [Sorry for the delay, also :)  (been sick/recovering)]
> 
> - Lifecycle: as long as the compositor can get a frame (as "current"), the chromium-side code keeps the backing texture in a state where it can be read from (and doesn't overwrite it with new data until it stops being gotten by the compositor).  I am not at all confident in this part of the CL.  Are you aware of any document/comments/anything that describes the intended lifecycle of LayerChromium's updateCompositorResources()/pushPropertiesTo() and CCLayerImpl's draw()?  (my impl cargo-culted what I found in  VideoLayer and PluginLayer).
> 

updateCompositorResources() may be called at "any" time on the WebKit main thread.  After that function is called, pushPropertiesTo() will be called within the same task.  After those two functions are called for a given LayerChromium, CCLayerImpl::draw() may be called on that layer one or more times, potentially from a different thread.  In particular, CCLayerImpl::draw() may be called one or more times *after* the associated LayerChromium is destroyed.  CCLayerImpl::draw() will not be called after the compositor itself is destroyed, which is usually when the tab is destroyed, the page is navigated, or no more composited content exists on the page.

> > As for (1) I'm not quite sure how it's supposed to work today. It seems that once a texture ID is grabbed from the VideoFrameProvider it is valid to bind and draw from, but for how long?
> 
> The provider tracks which frame is "current" and keeps it valid.
> (chromium CL is http://codereview.chromium.org/8686010/ in case that helps)

But when does the provider go down? Does the provider have the same lifetime as the VideoLayerChromium?  The draw() might be called after the layer is destroyed.

Additionally, we may call draw() one or more times after a new video frame is requested from the provider via updateCompositorResources(). Does the provider rely on requesting a frame meaning that the last frame will never be drawn again?  If so I think there's a problem there.

I think that in general it might be worth wiring the provider up closer to the draw().  One big advantage of this is that in threaded compositor mode the draw() function can be called while the WebKit main thread is blocked.  If we wire up new frame notifications and texture IDs directly to the drawing thread, we can keep video playing smoothly even if there's a long running block of JavaScript or layout or whatnot running.  I can give you some pointers to a solution that looks like this offline if you'd like.  Another advantage of moving closer to the draw() is that we can have more direct control over when certain texture IDs are used, etc.  A downside is that wiring up closer to the draw() means that multiple threads are involved, which complicated setup and shutdown.

[Ami Fischman]

Created attachment 119524 [details]
Patch

[Ami Fischman]

Thanks for dropping the knowledge bombs.

Please see the attached patch, which I believe implements all the lifecycle fixes we've been discussing.  VideoLayerChromium is gutted and almost all of its contents have migrated to CCVideoLayerImpl.
It is still test-free, but that's next!

[WebKit Review Bot]

Comment on attachment 119524 [details]
Patch

Attachment 119524 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10900448

[David Levin]

Comment on attachment 119524 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCVideoLayerImpl.cpp:97
> +    default:

If you're handling all values of VideoFrameChromium::Format, it would be better to get rid of the "default" and move the return outside of the switch so that appropriate warnings will kick in if more enum values are added.

If you aren't handing all values of VideoFrameChromium::Format, why not (to get the benefit of compile time checks when someone adds a new enum value)?

[Ami Fischman]

Created attachment 119549 [details]
Patch

[Ami Fischman]

(In reply to comment #28)
> If you're handling all values of VideoFrameChromium::Format, it would be better to get rid of the "default" and move the return outside of the switch so that appropriate warnings will kick in if more enum values are added.
> If you aren't handing all values of VideoFrameChromium::Format, why not (to get the benefit of compile time checks when someone adds a new enum value)?

Done.

[James Robinson]

Comment on attachment 119549 [details]
Patch

I'm not sure I understand the design around m_providerMutex. All of the functions it guards (~CCVideoLayerImpl, CCVideoLayerImpl::setProvider, CCVideoLayerImpl::draw) run on the same thread.

I'm also a bit uneasy about VideoLayerChromium holding a raw pointer to a CCVideoLayerImpl instance. There's another bit of logic elsewhere (that we may or may not keep) that drops the entire CC*LayerImpl tree when a tab goes to the background to save resources. Having direct pointers between the trees makes that sort of logic far more difficult.

Here's another suggestion for dealing with the provider shutdown issue that we use elsewhere in WebKit: declare a VideoFrameProviderClient interface with a void shutdown() function and add a setClient() call to VideoFrameProvider. Then maintain the invariant that whoever owns a pointer to a VideoFrameProvider has to set itself as the client and whenever a VideoFrameProvider instance goes away it needs to notify its client (a hand-rolled weak pointer). You will still need a bit of careful locking since during different points in the lifetime of the provider the client may live on different threads, but I think it's manageable.

[Ami Fischman]

(In reply to comment #31)
> (From update of attachment 119549 [details])
> I'm not sure I understand the design around m_providerMutex. All of the functions it guards (~CCVideoLayerImpl, CCVideoLayerImpl::setProvider, CCVideoLayerImpl::draw) run on the same thread.

No, the middle one (CCVideoLayerImpl::setProvider) is called by VideoLayerChromium::releaseProvider (called by ~WebMediaPlayerClientImpl), which runs on the main thread.

> I'm also a bit uneasy about VideoLayerChromium holding a raw pointer to a CCVideoLayerImpl instance. There's another bit of logic elsewhere (that we may or may not keep) that drops the entire CC*LayerImpl tree when a tab goes to the background to save resources. Having direct pointers between the trees makes that sort of logic far more difficult.
> Here's another suggestion for dealing with the provider shutdown issue that we use elsewhere in WebKit: declare a VideoFrameProviderClient interface with a void shutdown() function and add a setClient() call to VideoFrameProvider. Then maintain the invariant that whoever owns a pointer to a VideoFrameProvider has to set itself as the client and whenever a VideoFrameProvider instance goes away it needs to notify its client (a hand-rolled weak pointer). You will still need a bit of careful locking since during different points in the lifetime of the provider the client may live on different threads, but I think it's manageable.

I'll take a crack at this tomorrow morning.

[Ami Fischman]

Created attachment 119575 [details]
Patch

[Ami Fischman]

(In reply to comment #32)
> > Here's another suggestion for dealing with the provider shutdown issue that we use elsewhere in WebKit: declare a VideoFrameProviderClient interface with a void shutdown() function and add a setClient() call to VideoFrameProvider. Then maintain the invariant that whoever owns a pointer to a VideoFrameProvider has to set itself as the client and whenever a VideoFrameProvider instance goes away it needs to notify its client (a hand-rolled weak pointer). You will still need a bit of careful locking since during different points in the lifetime of the provider the client may live on different threads, but I think it's manageable.

Done.

[Ami Fischman]

Created attachment 119621 [details]
Patch

[Ami Fischman]

Created attachment 119628 [details]
Patch

[Ami Fischman]

Created attachment 119876 [details]
Patch

[Ami Fischman]

Comment on attachment 119876 [details]
Patch

Rebased to pick up r103086 (in preference to my revert of r102387).

[James Robinson]

Comment on attachment 119876 [details]
Patch

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

This looks good except for the case where the CCVideoLayerImpl dies before its provider. It looks like you have all the API surface necessary to handle this, but it just isn't quite wired up. R- for that and the "No new tests (OOPS!)." in the ChangeLog but other than that I think this is ready to land.

> Source/WebCore/ChangeLog:3
> +        Fix the life-cycle of video frames handled by VideoLayerChromium/CCVideoLayerImpl.

the preferred structure for ChangeLog entries is:

dateline

        one-line description of the patch (typically the bug title)
        https://bugs.webkit.org/show_bug.cgi?id=12345

        Reviewed by NOBODY (OOPS!).

        long form description of the patch, typically broken over
        multiple lines

        description of test coverage

        * list of files

> Source/WebCore/ChangeLog:14
> +        No new tests. (OOPS!)

this will fail an svn presubmit hook, you have to remove it from the ChangeLog entry yourself and replace it with a description of the test coverage for this patch (like the exiting compositing/video/ tests, for instance)

> Source/WebCore/platform/graphics/chromium/VideoLayerChromium.h:57
> +    virtual void stopUsingProvider();

nit: could you document that this is an implementation of the VideoFrameProvider::Client interface?

> Source/WebCore/platform/graphics/chromium/cc/CCVideoLayerImpl.cpp:74
> +    ASSERT(!m_provider); // Cleared during VideoLayerChromium::releaseProvider().

I believe this comment is out of date, since VideoLayerChromium::releaseProvider() isn't relevant to this codepath any more.

What is supposed to happen if the CCVideoLayerChromium is destroyed before the VideoFrameProvider? I would expect that cause the client to call setVideoFrameProviderClient(0) on the provider, but it doesn't seem that's what is happening here.

> Source/WebCore/platform/graphics/chromium/cc/CCVideoLayerImpl.cpp:87
> +static GC3Denum convertVFCFormatToGC3DFormat(VideoFrameChromium::Format vfcFormat)

we typically don't use abbreviations like 'vfc' in variable/parameter names in WebKit. Here I think that 'format' would be a fine variable name

> Source/WebCore/platform/graphics/chromium/cc/CCVideoLayerImpl.h:57
> +    virtual void stopUsingProvider();

could you document that this is an implementation of VideoFrameProvider::Client, and that it can be called from any thread?

> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:624
> +    client->m_videoFrameProviderClient = 0;

is there a strong reason to set this here instead of inside the WebMediaPlayerClientImpl c'tor?

> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.h:164
> +    Mutex m_compositingMutex; // Guards interaction between the main thread and compisiting thread.

typo 'compisiting'->'compositing'

ideally mutexes should guard data. could you document what data this mutex guards (it looks like it's guarding the m_videoFrameProviderClient pointer and potentially m_currentideoFrame, if I'm reading this correctly)

[Ami Fischman]

Created attachment 119959 [details]
Patch

Response to jamesr CR from #39

[James Robinson]

Comment on attachment 119959 [details]
Patch

Looks great, thanks for your persistence. R=me.

[WebKit Review Bot]

Comment on attachment 119959 [details]
Patch

Clearing flags on attachment: 119959

Committed r103298: <http://trac.webkit.org/changeset/103298>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.