Enable hole-punching in Android by default.

Enable hole-punching in Android by default.

This CL also enables the tests for hole-punching.

BUG=329447
R=boliu@chromium.org, qinmin@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269464

[benm (inactive), 2014-05-01 14:17:47]

https://codereview.chromium.org/264893002/diff/1/android_webview/javatests/sr...
File
android_webview/javatests/src/org/chromium/android_webview/test/ExternalVideoSurfaceContainerTest.java
(right):

https://codereview.chromium.org/264893002/diff/1/android_webview/javatests/sr...
android_webview/javatests/src/org/chromium/android_webview/test/ExternalVideoSurfaceContainerTest.java:104:
CommandLine.getInstance().appendSwitch("force-use-overlay-embedded-video");
Is this instead of calling
AwSettings.setVideoOverlayForEmbeddedVideoEnabled(true);

Would it be better to use that API as that's what embedders will do?

[ycheo (away), 2014-05-01 14:31:17]

https://codereview.chromium.org/264893002/diff/1/android_webview/javatests/sr...
File
android_webview/javatests/src/org/chromium/android_webview/test/ExternalVideoSurfaceContainerTest.java
(right):

https://codereview.chromium.org/264893002/diff/1/android_webview/javatests/sr...
android_webview/javatests/src/org/chromium/android_webview/test/ExternalVideoSurfaceContainerTest.java:104:
CommandLine.getInstance().appendSwitch("force-use-overlay-embedded-video");
On 2014/05/01 14:17:47, benm wrote:
> Is this instead of calling
> AwSettings.setVideoOverlayForEmbeddedVideoEnabled(true);
> 
> Would it be better to use that API as that's what embedders will do?

Currently, the hole-punching can be used only for the encrypted stream.
And because of some internal logic, we can't use the clear key media (it is
considered as unencrypted).  so the only remained way is to the widevine media. 
but, in case of WV, it needs some network communication to its backend, so IMO,
it's not appropriate for the test (What if the backend is down?).

Actually, I want to fix the fact that the clear key media is considered as
unencrypted, but the media team doesn't like the idea and this flag is created
to compensate it.

[benm (inactive), 2014-05-01 14:33:48]

On 2014/05/01 14:31:17, Yuncheol Heo wrote:
>
https://codereview.chromium.org/264893002/diff/1/android_webview/javatests/sr...
> File
>
android_webview/javatests/src/org/chromium/android_webview/test/ExternalVideoSurfaceContainerTest.java
> (right):
> 
>
https://codereview.chromium.org/264893002/diff/1/android_webview/javatests/sr...
>
android_webview/javatests/src/org/chromium/android_webview/test/ExternalVideoSurfaceContainerTest.java:104:
> CommandLine.getInstance().appendSwitch("force-use-overlay-embedded-video");
> On 2014/05/01 14:17:47, benm wrote:
> > Is this instead of calling
> > AwSettings.setVideoOverlayForEmbeddedVideoEnabled(true);
> > 
> > Would it be better to use that API as that's what embedders will do?
> 
> Currently, the hole-punching can be used only for the encrypted stream.
> And because of some internal logic, we can't use the clear key media (it is
> considered as unencrypted).  so the only remained way is to the widevine
media. 
> but, in case of WV, it needs some network communication to its backend, so
IMO,
> it's not appropriate for the test (What if the backend is down?).
> 
> Actually, I want to fix the fact that the clear key media is considered as
> unencrypted, but the media team doesn't like the idea and this flag is created
> to compensate it.

OK, and so I understand - setting the flag mean we don't need to call
AwSettings.setVideoOverlayForEmbeddedVideoEnabled(true)?

[ycheo (away), 2014-05-01 14:37:52]

On 2014/05/01 14:33:48, benm wrote:
> On 2014/05/01 14:31:17, Yuncheol Heo wrote:
> >
>
https://codereview.chromium.org/264893002/diff/1/android_webview/javatests/sr...
> > File
> >
>
android_webview/javatests/src/org/chromium/android_webview/test/ExternalVideoSurfaceContainerTest.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/264893002/diff/1/android_webview/javatests/sr...
> >
>
android_webview/javatests/src/org/chromium/android_webview/test/ExternalVideoSurfaceContainerTest.java:104:
> > CommandLine.getInstance().appendSwitch("force-use-overlay-embedded-video");
> > On 2014/05/01 14:17:47, benm wrote:
> > > Is this instead of calling
> > > AwSettings.setVideoOverlayForEmbeddedVideoEnabled(true);
> > > 
> > > Would it be better to use that API as that's what embedders will do?
> > 
> > Currently, the hole-punching can be used only for the encrypted stream.
> > And because of some internal logic, we can't use the clear key media (it is
> > considered as unencrypted).  so the only remained way is to the widevine
> media. 
> > but, in case of WV, it needs some network communication to its backend, so
> IMO,
> > it's not appropriate for the test (What if the backend is down?).
> > 
> > Actually, I want to fix the fact that the clear key media is considered as
> > unencrypted, but the media team doesn't like the idea and this flag is
created
> > to compensate it.
> 
> OK, and so I understand - setting the flag mean we don't need to call
> AwSettings.setVideoOverlayForEmbeddedVideoEnabled(true)?

Yes, as the flag name says it overrides all.

[boliu, 2014-05-01 16:34:45]

Min should know about this since this affects chrome on android as well.

I don't think this should land yet until you fix
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

[qinmin, 2014-05-01 17:07:13]

On 2014/05/01 16:34:45, boliu wrote:
> Min should know about this since this affects chrome on android as well.
> 
> I don't think this should land yet until you fix
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

I am confused by your comment, is this for another CL?

[boliu, 2014-05-01 17:08:39]

On 2014/05/01 17:07:13, qinmin wrote:
> On 2014/05/01 16:34:45, boliu wrote:
> > Min should know about this since this affects chrome on android as well.
> > 
> > I don't think this should land yet until you fix
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...
> 
> I am confused by your comment, is this for another CL?

Yes. I asked that turning on VIDEO_HOLE should not change chrome on android
behavior, and it's clearly not the case in that bit of code.

Probably should look through other places too

[ycheo (away), 2014-05-01 22:47:33]

On 2014/05/01 17:08:39, boliu wrote:
> On 2014/05/01 17:07:13, qinmin wrote:
> > On 2014/05/01 16:34:45, boliu wrote:
> > > Min should know about this since this affects chrome on android as well.
> > > 
> > > I don't think this should land yet until you fix
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...
> > 
> > I am confused by your comment, is this for another CL?
> 
> Yes. I asked that turning on VIDEO_HOLE should not change chrome on android
> behavior, and it's clearly not the case in that bit of code.
> 
> Probably should look through other places too

The job is clearly on my task list.
But, I didn't know that you want to clean this up before turning VIDEO_HOLE.
I'll prepare it right now.

[boliu, 2014-05-01 22:48:58]

On 2014/05/01 22:47:33, Yuncheol Heo wrote:
> The job is clearly on my task list.
> But, I didn't know that you want to clean this up before turning VIDEO_HOLE.
> I'll prepare it right now.

I don't want Min yelling at me or you for breaking chrome :)

[qinmin, 2014-05-01 23:04:44]

yes, chrome should never enable hole punching, only webview can, and probably on
certain platforms.

[ycheo (away), 2014-05-06 22:47:58]

Hi reviewers,

The cleaning job was finished.

Could you continue to review this CL?

Thanks,
Yuncheol

[boliu, 2014-05-07 00:08:09]

Just tried this and it doesn't compile in component build (android_clang_dbg
builds this configuration). You need to add media/media.gyp:media as an
dependency of android_webview_common.

Also webview in the old android browser with VIDEO_HOLE defined. It looks like
the inline video is not causing invalidations and depending the view to draw
itself. On www.videojs.com, if you start playing and lift your fingers, it will
stop drawing new video frames after a few seconds (the time it takes for the
invisible compositor scroll bars to hide), then if you scroll the webview, it
starts drawing again.

[ycheo (away), 2014-05-07 00:35:55]

On 2014/05/07 00:08:09, boliu wrote:
> Just tried this and it doesn't compile in component build (android_clang_dbg
> builds this configuration). You need to add media/media.gyp:media as an
> dependency of android_webview_common.
> 
> Also webview in the old android browser with VIDEO_HOLE defined. It looks like
> the inline video is not causing invalidations and depending the view to draw
> itself. On http://www.videojs.com, if you start playing and lift your fingers,
it will
> stop drawing new video frames after a few seconds (the time it takes for the
> invisible compositor scroll bars to hide), then if you scroll the webview, it
> starts drawing again.

Thanks for checking this out.

For the invalidation, isn't it the responsibility of surface flinger to merge
the surface view regardless of invalidations?

[boliu, 2014-05-07 00:37:52]

On 2014/05/07 00:35:55, Yuncheol Heo wrote:
> On 2014/05/07 00:08:09, boliu wrote:
> > Just tried this and it doesn't compile in component build (android_clang_dbg
> > builds this configuration). You need to add media/media.gyp:media as an
> > dependency of android_webview_common.
> > 
> > Also webview in the old android browser with VIDEO_HOLE defined. It looks
like
> > the inline video is not causing invalidations and depending the view to draw
> > itself. On http://www.videojs.com, if you start playing and lift your
fingers,
> it will
> > stop drawing new video frames after a few seconds (the time it takes for the
> > invisible compositor scroll bars to hide), then if you scroll the webview,
it
> > starts drawing again.
> 
> Thanks for checking this out.
> 
> For the invalidation, isn't it the responsibility of surface flinger to merge
> the surface view regardless of invalidations?

I only defined VIDEO_HOLE. I didn't turn on the run time toggle for hole
punching. So it should still be the existing path.

[ycheo (away), 2014-05-07 00:40:51]

On 2014/05/07 00:37:52, boliu wrote:
> On 2014/05/07 00:35:55, Yuncheol Heo wrote:
> > On 2014/05/07 00:08:09, boliu wrote:
> > > Just tried this and it doesn't compile in component build
(android_clang_dbg
> > > builds this configuration). You need to add media/media.gyp:media as an
> > > dependency of android_webview_common.
> > > 
> > > Also webview in the old android browser with VIDEO_HOLE defined. It looks
> like
> > > the inline video is not causing invalidations and depending the view to
draw
> > > itself. On http://www.videojs.com, if you start playing and lift your
> fingers,
> > it will
> > > stop drawing new video frames after a few seconds (the time it takes for
the
> > > invisible compositor scroll bars to hide), then if you scroll the webview,
> it
> > > starts drawing again.
> > 
> > Thanks for checking this out.
> > 
> > For the invalidation, isn't it the responsibility of surface flinger to
merge
> > the surface view regardless of invalidations?
> 
> I only defined VIDEO_HOLE. I didn't turn on the run time toggle for hole
> punching. So it should still be the existing path.

Got it.
I'll take a look at the issue, right now.

[ycheo (away), 2014-05-07 13:00:24]

On 2014/05/07 00:40:51, Yuncheol Heo wrote:
> On 2014/05/07 00:37:52, boliu wrote:
> > On 2014/05/07 00:35:55, Yuncheol Heo wrote:
> > > On 2014/05/07 00:08:09, boliu wrote:
> > > > Just tried this and it doesn't compile in component build
> (android_clang_dbg
> > > > builds this configuration). You need to add media/media.gyp:media as an
> > > > dependency of android_webview_common.
> > > > 
> > > > Also webview in the old android browser with VIDEO_HOLE defined. It
looks
> > like
> > > > the inline video is not causing invalidations and depending the view to
> draw
> > > > itself. On http://www.videojs.com, if you start playing and lift your
> > fingers,
> > > it will
> > > > stop drawing new video frames after a few seconds (the time it takes for
> the
> > > > invisible compositor scroll bars to hide), then if you scroll the
webview,
> > it
> > > > starts drawing again.
> > > 
> > > Thanks for checking this out.
> > > 
> > > For the invalidation, isn't it the responsibility of surface flinger to
> merge
> > > the surface view regardless of invalidations?
> > 
> > I only defined VIDEO_HOLE. I didn't turn on the run time toggle for hole
> > punching. So it should still be the existing path.
> 
> Got it.
> I'll take a look at the issue, right now.

@boliu,
I fixed the compile error in the component build:
https://codereview.chromium.org/253593002/

And for the invalidation issue, I also saw the issue at the time.
But, after syncing the repository to the latest, the issue was gone.
Could you retest it with the latest?

[ycheo (away), 2014-05-07 22:21:39]

On 2014/05/07 13:00:24, Yuncheol Heo wrote:
> On 2014/05/07 00:40:51, Yuncheol Heo wrote:
> > On 2014/05/07 00:37:52, boliu wrote:
> > > On 2014/05/07 00:35:55, Yuncheol Heo wrote:
> > > > On 2014/05/07 00:08:09, boliu wrote:
> > > > > Just tried this and it doesn't compile in component build
> > (android_clang_dbg
> > > > > builds this configuration). You need to add media/media.gyp:media as
an
> > > > > dependency of android_webview_common.
> > > > > 
> > > > > Also webview in the old android browser with VIDEO_HOLE defined. It
> looks
> > > like
> > > > > the inline video is not causing invalidations and depending the view
to
> > draw
> > > > > itself. On http://www.videojs.com, if you start playing and lift your
> > > fingers,
> > > > it will
> > > > > stop drawing new video frames after a few seconds (the time it takes
for
> > the
> > > > > invisible compositor scroll bars to hide), then if you scroll the
> webview,
> > > it
> > > > > starts drawing again.
> > > > 
> > > > Thanks for checking this out.
> > > > 
> > > > For the invalidation, isn't it the responsibility of surface flinger to
> > merge
> > > > the surface view regardless of invalidations?
> > > 
> > > I only defined VIDEO_HOLE. I didn't turn on the run time toggle for hole
> > > punching. So it should still be the existing path.
> > 
> > Got it.
> > I'll take a look at the issue, right now.
> 
> @boliu,
> I fixed the compile error in the component build:
> https://codereview.chromium.org/253593002/
> 
> And for the invalidation issue, I also saw the issue at the time.
> But, after syncing the repository to the latest, the issue was gone.
> Could you retest it with the latest?

Oh, the trybot message says there is another build error besides the component
build.
I'll take a look at it.
Thanks

[boliu, 2014-05-07 22:24:52]

On 2014/05/07 22:21:39, Yuncheol Heo wrote:
> Oh, the trybot message says there is another build error besides the component
> build.
> I'll take a look at it.
> Thanks

Oh, I didn't mean compile error in src/components when I said "component build".
See http://www.chromium.org/developers/how-tos/component-build and
https://code.google.com/p/chromium/codesearch#chromium/src/build/android/deve...

[ycheo (away), 2014-05-07 22:27:16]

Thanks for the info.
On May 8, 2014 7:24 AM, <boliu@chromium.org> wrote:

> On 2014/05/07 22:21:39, Yuncheol Heo wrote:
>
>> Oh, the trybot message says there is another build error besides the
>> component
>> build.
>> I'll take a look at it.
>> Thanks
>>
>
> Oh, I didn't mean compile error in src/components when I said "component
> build".
> See http://www.chromium.org/developers/how-tos/component-build and
> https://code.google.com/p/chromium/codesearch#chromium/
> src/build/android/developer_recommended_flags.gypi
>
> https://codereview.chromium.org/264893002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[boliu, 2014-05-08 01:00:30]

Btw I mentioned the fix for component build in #13 already.

I didn't have time to verify the invalidate issue today.

[ycheo (away), 2014-05-09 00:25:20]

On 2014/05/08 01:00:30, boliu wrote:
> Btw I mentioned the fix for component build in #13 already.
Sorry, I misunderstood your intention.
The fix is under CQ.

> 
> I didn't have time to verify the invalidate issue today.

Do you have any other concerns on this CL?
Actually, I'd like to land this CL at m36.

Thanks,
Yuncheol

[boliu, 2014-05-09 01:04:44]

I'm doing another round of manual testing. But Min should sign off on this too.

[boliu, 2014-05-09 01:13:08]

lgtm from me

wait for min though

[qinmin, 2014-05-09 01:14:11]

lgtm, please fix the try bot warning

[ycheo (away), 2014-05-09 10:17:13]

The CQ bit was checked by ycheo@chromium.org

[commit-bot: I haz the power, 2014-05-09 10:24:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ycheo@chromium.org/264893002/20001

[ycheo (away), 2014-05-09 23:18:36]

Committed patchset #2 manually as r269464 (presubmit successful).

[darin (slow to review), 2014-05-10 00:12:26]

https://codereview.chromium.org/264893002/diff/20001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/264893002/diff/20001/build/common.gypi#newcod...
build/common.gypi:682: 'video_hole%': 1,
I thought this was going to be enabled for only the WebView and not Chrome as
well. What's the story?

[boliu, 2014-05-10 00:19:02]

On 2014/05/10 00:12:26, darin wrote:
> https://codereview.chromium.org/264893002/diff/20001/build/common.gypi
> File build/common.gypi (right):
> 
>
https://codereview.chromium.org/264893002/diff/20001/build/common.gypi#newcod...
> build/common.gypi:682: 'video_hole%': 1,
> I thought this was going to be enabled for only the WebView and not Chrome as
> well. What's the story?

There is a run-time toggle that remains false on chrome on android.

The reason is in upstream chromium, webview is part of the regular android
build. So to write an end-to-end upstream, we have to define VIDEO_HOLE.

[ycheo (away), 2014-05-10 00:20:51]

On 2014/05/10 00:19:02, boliu wrote:
> On 2014/05/10 00:12:26, darin wrote:
> > https://codereview.chromium.org/264893002/diff/20001/build/common.gypi
> > File build/common.gypi (right):
> > 
> >
>
https://codereview.chromium.org/264893002/diff/20001/build/common.gypi#newcod...
> > build/common.gypi:682: 'video_hole%': 1,
> > I thought this was going to be enabled for only the WebView and not Chrome
as
> > well. What's the story?
> 
> There is a run-time toggle that remains false on chrome on android.
> 
> The reason is in upstream chromium, webview is part of the regular android
> build. So to write an end-to-end upstream, we have to define VIDEO_HOLE.

When I made the test for the hole-punching, I realized that there is no trybot
only for the WebView.
So I asked it to the WebView team.  and the WebView team said that it is
supposed to enable in the whole Android and Bart confirmed it.

[ycheo (away), 2014-05-10 00:23:11]

On 2014/05/10 00:20:51, Yuncheol Heo wrote:
> On 2014/05/10 00:19:02, boliu wrote:
> > On 2014/05/10 00:12:26, darin wrote:
> > > https://codereview.chromium.org/264893002/diff/20001/build/common.gypi
> > > File build/common.gypi (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/264893002/diff/20001/build/common.gypi#newcod...
> > > build/common.gypi:682: 'video_hole%': 1,
> > > I thought this was going to be enabled for only the WebView and not Chrome
> as
> > > well. What's the story?
> > 
> > There is a run-time toggle that remains false on chrome on android.
> > 
> > The reason is in upstream chromium, webview is part of the regular android
> > build. So to write an end-to-end upstream, we have to define VIDEO_HOLE.
> 
> When I made the test for the hole-punching, I realized that there is no trybot
> only for the WebView.
> So I asked it to the WebView team.  and the WebView team said that it is
> supposed to enable in the whole Android and Bart confirmed it.
The difference between WebView and Chrome for Android is whether to have an api
to enable the feature.

[darin (slow to review), 2014-05-10 03:13:04]

OK, thanks for the details!
-Darin


On Fri, May 9, 2014 at 5:23 PM, <ycheo@chromium.org> wrote:

> On 2014/05/10 00:20:51, Yuncheol Heo wrote:
>
>> On 2014/05/10 00:19:02, boliu wrote:
>> > On 2014/05/10 00:12:26, darin wrote:
>> > > https://codereview.chromium.org/264893002/diff/20001/
>> build/common.gypi
>> > > File build/common.gypi (right):
>> > >
>> > >
>> >
>>
>
> https://codereview.chromium.org/264893002/diff/20001/
> build/common.gypi#newcode682
>
>> > > build/common.gypi:682: 'video_hole%': 1,
>> > > I thought this was going to be enabled for only the WebView and not
>> Chrome
>> as
>> > > well. What's the story?
>> >
>> > There is a run-time toggle that remains false on chrome on android.
>> >
>> > The reason is in upstream chromium, webview is part of the regular
>> android
>> > build. So to write an end-to-end upstream, we have to define VIDEO_HOLE.
>>
>
>  When I made the test for the hole-punching, I realized that there is no
>> trybot
>> only for the WebView.
>> So I asked it to the WebView team.  and the WebView team said that it is
>> supposed to enable in the whole Android and Bart confirmed it.
>>
> The difference between WebView and Chrome for Android is whether to have
> an api
> to enable the feature.
>
>
> https://codereview.chromium.org/264893002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.