cc: Move video upload to VideoResourceUpdater.

cc: Move video upload to VideoResourceUpdater.

The VideoLayerImpl currently does upload of video frames from media::VideoFrame
to hardware/software resources, and displays these resources through quads. This
code is unfriendly to ubercomp as it requires holding onto the hardware resources
when using hardware decode, and because it reuses the same textures every frame
even though they need to be sent to the parent compositor.

This CL introduces the VideoResourceUpdater class, and moves all logic around
media::VideoFrame to that class. The VideoResourceUpdater class takes as input
a VideoFrame, and produces a set of TextureMailboxes for the video layer to
consume.

In the software case, the VideoResourceUpdater sets itself up as the release
callback so it can delete the backing texture when the browser compositor is
done with it (it could recycle it in the future).

In the hardware case, the VideoResourceUpdater takes a callback as input, so
it can have the WebMediaPlayer notified when the texture is given back from
the browser, and the hardware decoder can use it again for writing.

This CL also prepares us better for software uber-compositing video. The video
layer deals now only with abstract "resources", except for the mailboxes for
ubercompositor. The TextureMailbox construct needs to be abstracted in order to
hold a hardware or software backing for ubercompositor. Then the video layer's
special-case code for software can be entirely removed and it will be fully
software-uber-compositor-ready.

Currently, the VideoLayerImpl just makes use of the VideoResourceUpdater class,
making this a refactor without any functional change. This will make video
playback in ubercompositor work correctly for software-decoded video. In order
to address hardware-decoded video, a followup CL will have the WebMediaPlayer
hook up a callback through the VideoResourceUpdater to ensure it doesn't reuse
a texture for decode until it is returned from the parent compositor.

R=enne,jamesr
BUG=179729
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=193323

[danakj, 2013-04-06 02:31:28]

+piman for ResourceProvider: I've made it support WeakPtr so that I can tell
when it is gone. I did this so I can avoid leading resources when the
VideoLayerImpl is destroyed. It may have resources in the parent still, and when
they make it back, the callback function in VideoResourceUpdater needs to free
them. But in the lost-context case, the ResourceProvider is gone, so it needs to
know that and not call the deleted provider. This seemed like the cleanest way
to make that happen.

[danakj, 2013-04-06 02:44:35]

+skaslev FYI

[danakj, 2013-04-06 02:49:27]

There's something slightly off with software-compositing in this CL,
demonstrated by the layout test failures. I'll look into that another day, but
wanted to mention I saw it. Should be something small I think.

[danakj, 2013-04-07 18:58:32]

Sorted out software compositing in the latest patch.

[piman, 2013-04-08 21:03:47]

Some comments.

https://codereview.chromium.org/13445009/diff/23001/cc/resources/resource_pro...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/13445009/diff/23001/cc/resources/resource_pro...
cc/resources/resource_provider.h:47: : public
base::SupportsWeakPtr<ResourceProvider> {
Will the VideoResourceUpdater outlive the layer? It doesn't now but I'm not
entirely sure where this is going.
If it doesn't outlive the layer, then we can avoid this by relying on the
DidLoseOutputSurface callback in the layer.

I'm somewhat worried that serving a weak pointer here gives an easy way out, but
may prove problematic for cases where the RP gets destroyed without a context
loss (e.g. compositor teardown), but before the resources are properly released
- and it's then too late and we leak them.

https://codereview.chromium.org/13445009/diff/23001/cc/resources/video_resour...
File cc/resources/video_resource_updater.cc (right):

https://codereview.chromium.org/13445009/diff/23001/cc/resources/video_resour...
cc/resources/video_resource_updater.cc:130: // starts shaping up.
(not for this CL) would it make sense to move the yuv conversion to the
rendering?
Certainly there could be the concern of cost per video frame vs cost per
compositor frame, but it's similarly true on GL. Certainly the main use case we
want to optimize for is video playing, where we should have 1 compositor frame
per video frame.

https://codereview.chromium.org/13445009/diff/23001/cc/resources/video_resour...
cc/resources/video_resource_updater.cc:156: // were returned to us.
I think this is important. genMailboxCHROMIUM is a sync call (amortized, but
still), and I know the Android guys are worried about the cost of
gen/deleteTextures.

[danakj, 2013-04-08 21:44:56]

https://codereview.chromium.org/13445009/diff/23001/cc/resources/resource_pro...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/13445009/diff/23001/cc/resources/resource_pro...
cc/resources/resource_provider.h:47: : public
base::SupportsWeakPtr<ResourceProvider> {
Removed this as per offline convo. The callback is called from RP, so it doesn't
need to worry about getting called after the RP is destroyed.

https://codereview.chromium.org/13445009/diff/23001/cc/resources/video_resour...
File cc/resources/video_resource_updater.cc (right):

https://codereview.chromium.org/13445009/diff/23001/cc/resources/video_resour...
cc/resources/video_resource_updater.cc:130: // starts shaping up.
On 2013/04/08 21:03:47, piman wrote:
> (not for this CL) would it make sense to move the yuv conversion to the
> rendering?
> Certainly there could be the concern of cost per video frame vs cost per
> compositor frame, but it's similarly true on GL. Certainly the main use case
we
> want to optimize for is video playing, where we should have 1 compositor frame
> per video frame.

I think we should only be generating 1 mailbox video frame per compositor frame
also. It seems wasteful to generate more than one?

https://codereview.chromium.org/13445009/diff/23001/cc/resources/video_resour...
cc/resources/video_resource_updater.cc:156: // were returned to us.
On 2013/04/08 21:03:47, piman wrote:
> I think this is important. genMailboxCHROMIUM is a sync call (amortized, but
> still), and I know the Android guys are worried about the cost of
> gen/deleteTextures.

OK, I will do this in my next CL, is that sufficient? Filed
https://code.google.com/p/chromium/issues/detail?id=229065

[enne (OOO), 2013-04-09 20:54:34]

lgtm, now that I've had time to digest it.

I have the same concerns as piman about the change to now creating new textures
every frame for video, but fixing the code to reuse them in a followup seems
fine to me.

[commit-bot: I haz the power, 2013-04-09 20:56:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/13445009/27001

[commit-bot: I haz the power, 2013-04-09 20:56:26]

Presubmit check for 13445009-27001 failed and returned exit status 1.

INFO:root:Found 23 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/cc/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    content/common/cc_messages_unittest.cc
    content/common/cc_messages.h

Presubmit checks took 5.0s to calculate.

[danakj, 2013-04-09 20:57:52]

Ah, piman can you take a look at the content stuff, and I'll add a security
person.

[danakj, 2013-04-09 20:59:08]

+cdn for cc_messages.h

For video we're moving from passing resource id + size + format, to just passing
a resource id.

[Cris Neckar, 2013-04-09 21:25:30]

lgtm on IPC changes

[piman, 2013-04-09 23:19:17]

Yes, sorry, LGTM.

[commit-bot: I haz the power, 2013-04-09 23:20:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/13445009/27001

[danakj, 2013-04-09 23:25:25]

On Tue, Apr 9, 2013 at 4:19 PM, <piman@chromium.org> wrote:

> Yes, sorry, LGTM.
>

Cool, thanks.

[danakj, 2013-04-09 23:48:54]

@piman/@enne: please take a look at the last version. updated with lessons
learned from recycling. if we produce the hardware-path texture into a mailbox,
we should consume it before returning it to them.

[piman, 2013-04-10 00:58:44]

lgtm

[commit-bot: I haz the power, 2013-04-10 01:01:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/13445009/62001

[commit-bot: I haz the power, 2013-04-10 04:44:50]

Change committed as 193323