[Android WebView] Add tests to check whether the hole punching works.

[Android WebView] Add tests to check whether the hole punching works.

- Add ExternalVideoSurfaceContainerTest.
- Add a dependency fatory into ExternalVideoSurfaceContainer.
- Move AwSettingsTest.runVideoTest() into AwTestBase.

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

[ycheo (away), 2014-04-24 06:23:48]

PTAL.

[boliu, 2014-04-25 00:24:27]

https://codereview.chromium.org/250483002/diff/1/android_webview/java/src/org...
File
android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java
(right):

https://codereview.chromium.org/250483002/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:103:
return sFactory.create(nativeExternalVideoSurfaceContainer, contentViewCore);
Is sFactory going to be null in production?

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

https://codereview.chromium.org/250483002/diff/1/android_webview/javatests/sr...
android_webview/javatests/src/org/chromium/android_webview/test/ExternalVideoSurfaceContainerTest.java:118:
onPositionChangedCallCount, 150.0f, 150.0f);
It's not clear to me how you are getting the 150 value. Is that going to work
all devices, in all orientations?

[ycheo (away), 2014-04-25 01:33:00]

https://codereview.chromium.org/250483002/diff/1/android_webview/java/src/org...
File
android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java
(right):

https://codereview.chromium.org/250483002/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:103:
return sFactory.create(nativeExternalVideoSurfaceContainer, contentViewCore);
On 2014/04/25 00:24:27, boliu wrote:
> Is sFactory going to be null in production?

Oops! Thanks for checking this.

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

https://codereview.chromium.org/250483002/diff/1/android_webview/javatests/sr...
android_webview/javatests/src/org/chromium/android_webview/test/ExternalVideoSurfaceContainerTest.java:118:
onPositionChangedCallCount, 150.0f, 150.0f);
On 2014/04/25 00:24:27, boliu wrote:
> It's not clear to me how you are getting the 150 value. Is that going to work
> all devices, in all orientations?

I've tested this on N5 and N7.
And the actually the media is 150x150, not the same as its description:

ycheo@xelnaga:~$ avprobe one_pixel_one_frawe.webm 
avprobe version 0.8.10-4:0.8.10-0ubuntu0.12.04.1, Copyright (c) 2007-2013 the
Libav developers
  built on Feb  6 2014 20:56:59 with gcc 4.6.3
[matroska,webm @ 0x224e7a0] Unknown entry 0x63C5
[matroska,webm @ 0x224e7a0] Estimating duration from bitrate, this may be
inaccurate
Input #0, matroska,webm, from 'one_pixel_one_frawe.webm':
  Duration: 00:00:01.00, start: 0.000000, bitrate: N/A
    Stream #0.0: Video: vp8, yuv420p, 150x150, PAR 1:1 DAR 1:1, 1k tbr, 1k tbn,
1k tbc (default)

[boliu, 2014-04-25 15:17:27]

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

https://codereview.chromium.org/250483002/diff/1/android_webview/javatests/sr...
android_webview/javatests/src/org/chromium/android_webview/test/ExternalVideoSurfaceContainerTest.java:118:
onPositionChangedCallCount, 150.0f, 150.0f);
On 2014/04/25 01:33:00, Yuncheol Heo wrote:
> On 2014/04/25 00:24:27, boliu wrote:
> > It's not clear to me how you are getting the 150 value. Is that going to
work
> > all devices, in all orientations?
> 
> I've tested this on N5 and N7.
> And the actually the media is 150x150, not the same as its description:
> 
> ycheo@xelnaga:~$ avprobe one_pixel_one_frawe.webm 
> avprobe version 0.8.10-4:0.8.10-0ubuntu0.12.04.1, Copyright (c) 2007-2013 the
> Libav developers
>   built on Feb  6 2014 20:56:59 with gcc 4.6.3
> [matroska,webm @ 0x224e7a0] Unknown entry 0x63C5
> [matroska,webm @ 0x224e7a0] Estimating duration from bitrate, this may be
> inaccurate
> Input #0, matroska,webm, from 'one_pixel_one_frawe.webm':
>   Duration: 00:00:01.00, start: 0.000000, bitrate: N/A
>     Stream #0.0: Video: vp8, yuv420p, 150x150, PAR 1:1 DAR 1:1, 1k tbr, 1k
tbn,
> 1k tbc (default)

Is the size here not affected by device screen density? Shouldn't the size be
screen pixels. Might just be an accident. I'd much rather have this being
explicitly worked out, so need to set the appropriate viewport tag + css
property.

From https://developer.chrome.com/multidevice/webview/pixelperfect, I think
you'll need, <meta name="viewport" content="width=device-width,
initial-scale=1">, then set the element width and height to 150px. +mnaganov to
check that.

[mnaganov (inactive), 2014-04-25 15:29:17]

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

https://codereview.chromium.org/250483002/diff/1/android_webview/javatests/sr...
android_webview/javatests/src/org/chromium/android_webview/test/ExternalVideoSurfaceContainerTest.java:118:
onPositionChangedCallCount, 150.0f, 150.0f);
On 2014/04/25 15:17:28, boliu wrote:
> On 2014/04/25 01:33:00, Yuncheol Heo wrote:
> > On 2014/04/25 00:24:27, boliu wrote:
> > > It's not clear to me how you are getting the 150 value. Is that going to
> work
> > > all devices, in all orientations?
> > 
> > I've tested this on N5 and N7.
> > And the actually the media is 150x150, not the same as its description:
> > 
> > ycheo@xelnaga:~$ avprobe one_pixel_one_frawe.webm 
> > avprobe version 0.8.10-4:0.8.10-0ubuntu0.12.04.1, Copyright (c) 2007-2013
the
> > Libav developers
> >   built on Feb  6 2014 20:56:59 with gcc 4.6.3
> > [matroska,webm @ 0x224e7a0] Unknown entry 0x63C5
> > [matroska,webm @ 0x224e7a0] Estimating duration from bitrate, this may be
> > inaccurate
> > Input #0, matroska,webm, from 'one_pixel_one_frawe.webm':
> >   Duration: 00:00:01.00, start: 0.000000, bitrate: N/A
> >     Stream #0.0: Video: vp8, yuv420p, 150x150, PAR 1:1 DAR 1:1, 1k tbr, 1k
> tbn,
> > 1k tbc (default)
> 
> Is the size here not affected by device screen density? Shouldn't the size be
> screen pixels. Might just be an accident. I'd much rather have this being
> explicitly worked out, so need to set the appropriate viewport tag + css
> property.
> 
> From https://developer.chrome.com/multidevice/webview/pixelperfect, I think
> you'll need, <meta name="viewport" content="width=device-width,
> initial-scale=1">, then set the element width and height to 150px. +mnaganov
to
> check that.

Why should the effective size be affected by density? I guess, videos should be
treated the same way as images -- the size of the img element is set to CSS
pixels from image dimensions in pixels.

[boliu, 2014-04-25 15:31:51]

On 2014/04/25 15:29:17, Mikhail Naganov (Cr) wrote:
> Why should the effective size be affected by density? I guess, videos should
be
> treated the same way as images -- the size of the img element is set to CSS
> pixels from image dimensions in pixels.

Is that always a safe default assumption? If so then I'm fine with this.

[mnaganov (inactive), 2014-04-25 15:43:52]

On 2014/04/25 15:31:51, boliu wrote:
> On 2014/04/25 15:29:17, Mikhail Naganov (Cr) wrote:
> > Why should the effective size be affected by density? I guess, videos should
> be
> > treated the same way as images -- the size of the img element is set to CSS
> > pixels from image dimensions in pixels.
> 
> Is that always a safe default assumption? If so then I'm fine with this.

It looks so. Here
(http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element...):

"The intrinsic width and intrinsic height of the media resource are the
dimensions of the resource in CSS pixels after taking into account the
resource's dimensions, aspect ratio, clean aperture, resolution, and so forth,
as defined for the format used by the resource."

Nothing says that screen density is taken into account. And HTML elements
dimensions are always in CSS pixels.


In fact, these "pixel perfect" tricks are needed when you need to make sure that
a pixel of your image / video matches exactly one screen pixel, e.g. when
scaling an image can introduce a moire pattern.

[boliu, 2014-04-25 16:03:21]

On 2014/04/25 15:43:52, Mikhail Naganov (Cr) wrote:
> On 2014/04/25 15:31:51, boliu wrote:
> > On 2014/04/25 15:29:17, Mikhail Naganov (Cr) wrote:
> > > Why should the effective size be affected by density? I guess, videos
should
> > be
> > > treated the same way as images -- the size of the img element is set to
CSS
> > > pixels from image dimensions in pixels.
> > 
> > Is that always a safe default assumption? If so then I'm fine with this.
> 
> It looks so. Here
>
(http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element...):
> 
> "The intrinsic width and intrinsic height of the media resource are the
> dimensions of the resource in CSS pixels after taking into account the
> resource's dimensions, aspect ratio, clean aperture, resolution, and so forth,
> as defined for the format used by the resource."
> 
> Nothing says that screen density is taken into account. And HTML elements
> dimensions are always in CSS pixels.
> 

I'm not really parsing da spec...and scaling is hard...

It's describing the "intrinsic width/height" here. Let's use an example. In this
test, the video is 150x150 pixels, and on a nexus 5, the device density is 3.
Does that mean the instrinsic width/height of this video is 150 / 3 = 50 css
pixels?

Then is it a safe assumption that with no styles applied, the intrinsic
width/height will be used as width/heigh in layout? (which means the test should
get back 150 in physical pixels).

> 
> In fact, these "pixel perfect" tricks are needed when you need to make sure
that
> a pixel of your image / video matches exactly one screen pixel, e.g. when
> scaling an image can introduce a moire pattern.

"tricks are needed" or "tricks are not needed"?

That's exactly what I'm trying to make sure. The video itself is 150x150 pixels,
and the test should be reading the video element width/height in screen pixels.
I was a bit surprised that they actually match.

[mnaganov (inactive), 2014-04-25 16:25:43]

On 2014/04/25 16:03:21, boliu wrote:
> On 2014/04/25 15:43:52, Mikhail Naganov (Cr) wrote:
> > On 2014/04/25 15:31:51, boliu wrote:
> > > On 2014/04/25 15:29:17, Mikhail Naganov (Cr) wrote:
> > > > Why should the effective size be affected by density? I guess, videos
> should
> > > be
> > > > treated the same way as images -- the size of the img element is set to
> CSS
> > > > pixels from image dimensions in pixels.
> > > 
> > > Is that always a safe default assumption? If so then I'm fine with this.
> > 
> > It looks so. Here
> >
>
(http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element...):
> > 
> > "The intrinsic width and intrinsic height of the media resource are the
> > dimensions of the resource in CSS pixels after taking into account the
> > resource's dimensions, aspect ratio, clean aperture, resolution, and so
forth,
> > as defined for the format used by the resource."
> > 
> > Nothing says that screen density is taken into account. And HTML elements
> > dimensions are always in CSS pixels.
> > 
> 
> I'm not really parsing da spec...and scaling is hard...
> 
> It's describing the "intrinsic width/height" here. Let's use an example. In
this
> test, the video is 150x150 pixels, and on a nexus 5, the device density is 3.
> Does that mean the instrinsic width/height of this video is 150 / 3 = 50 css
> pixels?
> 

No. Intrinsic (aka "natual") size of an image or video is whatever it says in
its dimensions. Thus, an embedded image or video that says it is 150 x 150 will
make <img> / <video> element to become 150 x 150 CSS pixels size.

> Then is it a safe assumption that with no styles applied, the intrinsic
> width/height will be used as width/heigh in layout? (which means the test
should
> get back 150 in physical pixels).
> 
> > 
> > In fact, these "pixel perfect" tricks are needed when you need to make sure
> that
> > a pixel of your image / video matches exactly one screen pixel, e.g. when
> > scaling an image can introduce a moire pattern.
> 
> "tricks are needed" or "tricks are not needed"?
>

Needed. If you load an image having 150x150 pixels size on a hi-res display, it
will be displayed as 150x150 CSS pixels, thus scaled by the screen density
factor. If you want to avoid scaling, you provide an image of (150 * density) x
(150 * density) size and then set <img width=150 height=150>, so each pixel of
the image will occupy one screen pixel.
 
> That's exactly what I'm trying to make sure. The video itself is 150x150
pixels,
> and the test should be reading the video element width/height in screen
pixels.
> I was a bit surprised that they actually match.

OnExternalVideoSurfacePositionChanged provides dimensions in CSS pixels:

  // Called when the position and size of the video element which uses
  // the external video surface is changed.
  // |rect| contains the new position and size in css pixels.
  virtual void OnExternalVideoSurfacePositionChanged(
      int player_id, const gfx::RectF& rect) = 0;

What makes you think the test reads sizes in screen pixels?

[boliu, 2014-04-25 16:44:31]

On 2014/04/25 16:25:43, Mikhail Naganov (Cr) wrote:
> What makes you think the test reads sizes in screen pixels?

From memory..? My bad. That's where my confusion came from. Thanks for the
explanation.

So now I've got a few other objections.

One is these tests are testing logic in content and the bit of hook up in
android_webview. This suggests these tests should really be in content layer.
That move can wait until VIDEO_HOLE is turned on. But the bigger issue is these
tests do not exercise any logic in android_webview, which kinda loses the whole
point.

Anyway, this CL lgtm, but a bunch of follow ups.

[ycheo (away), 2014-04-28 02:19:24]

On 2014/04/25 16:44:31, boliu wrote:
Thanks Mikhail for the detail explanation.
Thanks Bo for the review.

> On 2014/04/25 16:25:43, Mikhail Naganov (Cr) wrote:
> > What makes you think the test reads sizes in screen pixels?
> 
> From memory..? My bad. That's where my confusion came from. Thanks for the
> explanation.
> 
> So now I've got a few other objections.
> 
> One is these tests are testing logic in content and the bit of hook up in
> android_webview. This suggests these tests should really be in content layer.
> That move can wait until VIDEO_HOLE is turned on. 
IMO, what you want is the different test with this.
This is a test that Java object gets the appropriate calls.
For testing in content, it should be a C level test.

> But the bigger issue is these
> tests do not exercise any logic in android_webview, which kinda loses the
whole
> point.
I'll prepare this first.

> 
> Anyway, this CL lgtm, but a bunch of follow ups.

[ycheo (away), 2014-04-29 00:51:20]

The CQ bit was checked by ycheo@chromium.org

[commit-bot: I haz the power, 2014-04-29 00:52:01]

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

[commit-bot: I haz the power, 2014-04-29 01:27:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-29 01:27:53]

Try jobs failed on following builders:
  tryserver.chromium on chromium_presubmit

[ycheo (away), 2014-04-29 02:35:00]

The CQ bit was checked by ycheo@chromium.org

[commit-bot: I haz the power, 2014-04-29 02:35:50]

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

[commit-bot: I haz the power, 2014-04-29 03:31:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-29 03:31:04]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[ycheo (away), 2014-04-30 07:42:44]

The CQ bit was checked by ycheo@chromium.org

[commit-bot: I haz the power, 2014-04-30 07:43:27]

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

[commit-bot: I haz the power, 2014-04-30 07:58:32]

Change committed as 267125