Ties RenderFrameImpl VIDEO_HOLE observer to lifetime of RenderWidget.

Ties RenderFrameImpl VIDEO_HOLE observer to lifetime of RenderWidget.

This allows using the VIDEO_HOLE outside of Android.

R=xhwang,damienv1
BUG=392993
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282783

[xhwang, 2014-07-08 00:02:43]

looking good to me. I'd defer to ycheo@ to review this CL since he's more
familiar with VIDEO_HOLE code.

https://codereview.chromium.org/375713002/diff/1/content/renderer/render_fram...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/375713002/diff/1/content/renderer/render_fram...
content/renderer/render_frame_impl.cc:3565: }
nit: no need for {} anymore.

[gunsch, 2014-07-08 00:06:23]

https://codereview.chromium.org/375713002/diff/1/content/renderer/render_fram...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/375713002/diff/1/content/renderer/render_fram...
content/renderer/render_frame_impl.cc:3565: }
On 2014/07/08 00:02:43, xhwang wrote:
> nit: no need for {} anymore.

Done.

[gunsch, 2014-07-09 16:21:02]

Yungcheol, ping?

[ycheo (away), 2014-07-09 21:04:40]

https://codereview.chromium.org/375713002/diff/20001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/375713002/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:421:
render_view_->RegisterVideoHoleFrame(this);
Do you have any specific reason to register the observer always?

[gunsch-google, 2014-07-09 21:12:43]

https://codereview.chromium.org/375713002/diff/20001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/375713002/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:421:
render_view_->RegisterVideoHoleFrame(this);
On 2014/07/09 21:04:39, Yuncheol Heo wrote:
> Do you have any specific reason to register the observer always? 

No technical reason, but it seemed reasonable to tie it to the lifetime of
RenderFrameImpl.

Would you prefer to register the observer in the first call to
createMediaPlayer, then presumably have a flag to track?

[ycheo (away), 2014-07-09 21:31:53]

On 2014/07/09 21:12:43, gunsch-google wrote:
>
https://codereview.chromium.org/375713002/diff/20001/content/renderer/render_...
> File content/renderer/render_frame_impl.cc (right):
> 
>
https://codereview.chromium.org/375713002/diff/20001/content/renderer/render_...
> content/renderer/render_frame_impl.cc:421:
> render_view_->RegisterVideoHoleFrame(this);
> On 2014/07/09 21:04:39, Yuncheol Heo wrote:
> > Do you have any specific reason to register the observer always? 
> 
> No technical reason, but it seemed reasonable to tie it to the lifetime of
> RenderFrameImpl.
> 
> Would you prefer to register the observer in the first call to
> createMediaPlayer, then presumably have a flag to track?

+xhwang,
The code was added by you.
Which one do you prefer?

[xhwang, 2014-07-09 23:52:08]

https://codereview.chromium.org/375713002/diff/20001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/375713002/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:421:
render_view_->RegisterVideoHoleFrame(this);
On 2014/07/09 21:12:43, gunsch-google wrote:
> On 2014/07/09 21:04:39, Yuncheol Heo wrote:
> > Do you have any specific reason to register the observer always? 
> 
> No technical reason, but it seemed reasonable to tie it to the lifetime of
> RenderFrameImpl.
> 
> Would you prefer to register the observer in the first call to
> createMediaPlayer, then presumably have a flag to track?

Register in createMediaPlayer() sgtm. Actually create a RenderFrameObserver for
registering VideoHoleFrame so that in ~RenderFrameImpl, the unregistering will
happen automatically?

[gunsch, 2014-07-10 00:42:58]

https://codereview.chromium.org/375713002/diff/20001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/375713002/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:421:
render_view_->RegisterVideoHoleFrame(this);
On 2014/07/09 23:52:08, xhwang wrote:
> On 2014/07/09 21:12:43, gunsch-google wrote:
> > On 2014/07/09 21:04:39, Yuncheol Heo wrote:
> > > Do you have any specific reason to register the observer always? 
> > 
> > No technical reason, but it seemed reasonable to tie it to the lifetime of
> > RenderFrameImpl.
> > 
> > Would you prefer to register the observer in the first call to
> > createMediaPlayer, then presumably have a flag to track?
> 
> Register in createMediaPlayer() sgtm. Actually create a RenderFrameObserver
for
> registering VideoHoleFrame so that in ~RenderFrameImpl, the unregistering will
> happen automatically?

Discussed offline, that seems to be excessive complexity.

Moved to registering only once, in createMediaPlayer, tracked with a flag.

[xhwang, 2014-07-10 00:44:25]

lgtm

[ycheo (away), 2014-07-10 00:59:38]

lgtm

[gunsch, 2014-07-10 16:41:31]

Just noticed jam@ is OOO, sending to jamesr@ for OWNERS

[jamesr, 2014-07-10 21:33:43]

Why?  I don't see a bug or a motivation in the change description.

[gunsch, 2014-07-10 21:42:14]

On 2014/07/10 21:33:43, jamesr wrote:
> Why?  I don't see a bug or a motivation in the change description.

Hmm, I thought "This allows using the VIDEO_HOLE outside of Android." described
a motivation. There's no reason that VIDEO_HOLE and OS_ANDROID ought to be
coupled, since they are two separate defines controlled by their own GYP
variables, but the code as written in this file was coupled with that
assumption. Chromecast also uses VIDEO_HOLE.

I've filed an issue stating as much and attached to this CL.

[jamesr, 2014-07-10 21:44:02]

The VIDEO_HOLE stuff was landed with a promise that it'd only be used by
android, as I recall.

[gunsch, 2014-07-10 21:56:11]

On 2014/07/10 21:44:02, jamesr wrote:
> The VIDEO_HOLE stuff was landed with a promise that it'd only be used by
> android, as I recall.

How would you like to proceed from here, then? I don't know about early
decisions regarding VIDEO_HOLE, but I do know that Chromecast is using it out of
a similar need: we do all of our video rendering through hardware with the
assumption that we have hole-punching. See:

https://code.google.com/p/chromium/codesearch#chromium/src/build/common.gypi&...

[jamesr, 2014-07-10 21:57:25]

I don't know what the plans for chromecast are.  The video hole code doesn't
work with any of the rest of our compositor stack and can't be supported in
general.

[damienv1, 2014-07-10 22:04:36]

James, Chromecast has the same constraints as Android on this side. We totally
understand that this is not supported by the Chrome compositor team. If we could
do in different way, we would do it but the graphics performance on Chromecast
are just not enough to render 1080p30 video through the software or the GL
rendering path (and we also have the same constraints as Android for encrypted
content).

This CL just makes the VIDEO_HOLE a feature that can be used by the chromecast
platform (no impact on the rest of the code).
Thanks for your feedback.

[jamesr, 2014-07-12 00:04:38]

reluctant lgtm

[gunsch, 2014-07-12 00:10:30]

The CQ bit was checked by gunsch@chromium.org

[commit-bot: I haz the power, 2014-07-12 00:11:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/gunsch@chromium.org/375713002/40001

[commit-bot: I haz the power, 2014-07-12 02:27:27]

Change committed as 282783