Plumb through cropped output size for VideoFrame

Plumb through cropped output size for VideoFrame

The video playback path needs to be able to handle cropped video frames,
e.g. for HW decoders that output to macroblock-rounded buffer sizes.

* Composite only the visible subrect from WebVideoFrame in
  cc::VideoLayerImpl
* Remove some extraneous cropping logic from cc::VideoLayerImpl now that
  we have exact cropping info.
* media::VideoFrame replaces "data_size_" member with "coded_size_", and
  adds a "visible_rect_" gfx::Rect to indicate the sub-rect of the
  entire frame that should be visible (after cropping)
* Associated changes to various decoder/capture pipelines to plumb this
  through.

TEST=build, run on x86, ARM
BUG=155416, 140509, chrome-os-partner:15230
Change-Id: I284bc893959db427bc9ae677aed8b07292d228ae

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=166988

[sheu, 2012-10-24 22:49:36]

PTAL.  A lot of one-line-in-file changes due to changing interface.

[sheu, 2012-10-24 22:50:20]

Also: I'm not sure how to test the YUV path.  Suggestions welcome.

[jamesr, 2012-10-25 00:26:34]

Some style nits - I think Ami or someone else more familiar with the video stack
should probably do the review.

https://chromiumcodereview.appspot.com/11269017/diff/1/cc/video_layer_impl.cc
File cc/video_layer_impl.cc (right):

https://chromiumcodereview.appspot.com/11269017/diff/1/cc/video_layer_impl.cc...
cc/video_layer_impl.cc:127: DCHECK(m_frame->visibleX() == 0 &&
m_frame->visibleY() == 0);
Can you make these separate DCHECK()s and use DCHECK_EQ() so if they do fail
it'll be possible to see what is actually different?

https://chromiumcodereview.appspot.com/11269017/diff/1/cc/video_layer_impl.cc...
cc/video_layer_impl.cc:310: m_frame->data(planeIdx));
General note for stuff in src/cc/ - we're still using 4-space indents here and
need to reformat, so don't worry about being too careful to hit 80col if it
breaks the line in an odd place.  we'll bulk reformat the whole directory at
some point

https://chromiumcodereview.appspot.com/11269017/diff/1/cc/video_layer_impl.cc...
cc/video_layer_impl.cc:313: DCHECK(plane.format == GL_LUMINANCE);
DCHECK_EQ please, it'll print the format if the check fails

[Ami GONE FROM CHROMIUM, 2012-10-25 00:43:07]

I <3 this!

I'm not competent to review anything involving GL, though.  If enne@ begs off
like jamesr@ did I suggest you make the two of them do freestyle rap battle;
loser has to review.

https://chromiumcodereview.appspot.com/11269017/diff/1/cc/layer_tree_host_imp...
File cc/layer_tree_host_impl_unittest.cc (right):

https://chromiumcodereview.appspot.com/11269017/diff/1/cc/layer_tree_host_imp...
cc/layer_tree_host_impl_unittest.cc:2551: 
y u no test the interesting case of non-zero offset and non-1.0 scaling?

https://chromiumcodereview.appspot.com/11269017/diff/1/cc/video_layer_impl.cc
File cc/video_layer_impl.cc (right):

https://chromiumcodereview.appspot.com/11269017/diff/1/cc/video_layer_impl.cc...
cc/video_layer_impl.cc:127: DCHECK(m_frame->visibleX() == 0 &&
m_frame->visibleY() == 0);
DCHECKs are for programming errors, not creatively produced files downloaded
from the web.
If a file from the web can end up triggering this then it should be an error-out
condition.

https://chromiumcodereview.appspot.com/11269017/diff/1/cc/video_layer_impl.cc...
cc/video_layer_impl.cc:129: // The visible contents might not cover the entire
texture; e.g. padding
This comment seems oddly placed.

https://chromiumcodereview.appspot.com/11269017/diff/1/cc/video_layer_impl.cc...
cc/video_layer_impl.cc:184: // YUV software decoder.
I don't believe it's possible to get here with scales != 1.
DCHECK that?

https://chromiumcodereview.appspot.com/11269017/diff/1/media/base/video_frame.cc
File media/base/video_frame.cc (right):

https://chromiumcodereview.appspot.com/11269017/diff/1/media/base/video_frame...
media/base/video_frame.cc:60: natural_size.width() * natural_size.height() <=
limits::kMaxCanvas);
Seems like there's an opp for a helper static bool method here ;)

https://chromiumcodereview.appspot.com/11269017/diff/1/media/filters/skcanvas...
File media/filters/skcanvas_video_renderer.cc (right):

https://chromiumcodereview.appspot.com/11269017/diff/1/media/filters/skcanvas...
media/filters/skcanvas_video_renderer.cc:126:
video_frame->visible_rect().height() / local_dest_irect_saved.height();
Can you add testing for this stuff in the unittest for this file?

https://chromiumcodereview.appspot.com/11269017/diff/1/media/filters/skcanvas...
media/filters/skcanvas_video_renderer.cc:144: // TODO(hcfam): do rotation and
mirroring here.
lolwat

https://chromiumcodereview.appspot.com/11269017/diff/1/media/filters/skcanvas...
media/filters/skcanvas_video_renderer.cc:188: media::YUVType yuv_type =
(video_frame->format() == media::VideoFrame::YV12) ?
This thunk is wrong for the native texture case, where none of it is relevant. 
I think you want to move it into the IsEitherYV12OrYV16 block at l.212

https://chromiumcodereview.appspot.com/11269017/diff/1/media/filters/skcanvas...
media/filters/skcanvas_video_renderer.cc:208:
video_frame->data(media::VideoFrame::kVPlane) + uv_offset;
I think this thunk is correct (% needing to move per above), but it *definitely*
wants to be tested ;)

https://chromiumcodereview.appspot.com/11269017/diff/1/webkit/media/webvideof...
File webkit/media/webvideoframe_impl.cc (right):

https://chromiumcodereview.appspot.com/11269017/diff/1/webkit/media/webvideof...
webkit/media/webvideoframe_impl.cc:88: if (video_frame_.get())
flip the tests here and below to be consistent with the remaining methods above.

[jamesr, 2012-10-25 01:02:47]

If you can explain how the code deleted from the GL paths is accounted for
elsewhere I can review that - a lot of the logic appears to just be moving
around and it's hard for me to figure out where it's going now.  Maybe you could
write a chunk-by-chunk guide?  There don't seem to be too many pieces moving
around, but slightly more than I can keep track of

[sheu, 2012-10-25 01:53:53]

Updated, PTAL.

I haven't added tests yet to SkCanvas.  jamesr@: what do you mean by the code
removed from the GL path?  I removed some in VideoLayerImpl, but that was from
the YUV CPU upload path.

https://chromiumcodereview.appspot.com/11269017/diff/1/cc/layer_tree_host_imp...
File cc/layer_tree_host_impl_unittest.cc (right):

https://chromiumcodereview.appspot.com/11269017/diff/1/cc/layer_tree_host_imp...
cc/layer_tree_host_impl_unittest.cc:2551: 
On 2012/10/25 00:43:07, Ami Fischman wrote:
> y u no test the interesting case of non-zero offset and non-1.0 scaling?

Done.

https://chromiumcodereview.appspot.com/11269017/diff/1/cc/video_layer_impl.cc
File cc/video_layer_impl.cc (right):

https://chromiumcodereview.appspot.com/11269017/diff/1/cc/video_layer_impl.cc...
cc/video_layer_impl.cc:127: DCHECK(m_frame->visibleX() == 0 &&
m_frame->visibleY() == 0);
Updated the comment a bit.  It's not really an error, it's just that we've never
handled that case before and I'd like to explicitly document that.  We could
easily add the code to handle it, but there's not been a requirement yet.

https://chromiumcodereview.appspot.com/11269017/diff/1/cc/video_layer_impl.cc...
cc/video_layer_impl.cc:129: // The visible contents might not cover the entire
texture; e.g. padding
On 2012/10/25 00:43:07, Ami Fischman wrote:
> This comment seems oddly placed.

Done.

https://chromiumcodereview.appspot.com/11269017/diff/1/cc/video_layer_impl.cc...
cc/video_layer_impl.cc:184: // YUV software decoder.
It's not an error condition though, and we can handle it.

https://chromiumcodereview.appspot.com/11269017/diff/1/cc/video_layer_impl.cc...
cc/video_layer_impl.cc:310: m_frame->data(planeIdx));
On 2012/10/25 00:26:34, jamesr wrote:
> General note for stuff in src/cc/ - we're still using 4-space indents here and
> need to reformat, so don't worry about being too careful to hit 80col if it
> breaks the line in an odd place.  we'll bulk reformat the whole directory at
> some point

Done.

https://chromiumcodereview.appspot.com/11269017/diff/1/cc/video_layer_impl.cc...
cc/video_layer_impl.cc:313: DCHECK(plane.format == GL_LUMINANCE);
On 2012/10/25 00:26:34, jamesr wrote:
> DCHECK_EQ please, it'll print the format if the check fails

Done.

https://chromiumcodereview.appspot.com/11269017/diff/1/media/base/video_frame.cc
File media/base/video_frame.cc (right):

https://chromiumcodereview.appspot.com/11269017/diff/1/media/base/video_frame...
media/base/video_frame.cc:60: natural_size.width() * natural_size.height() <=
limits::kMaxCanvas);
This kinda _is_ the helper static bool?

I mean I guess I could break out another function so that you could (yo dawg I
herd u liek to) helper while you helper, but that seems excessive.

https://chromiumcodereview.appspot.com/11269017/diff/1/media/filters/skcanvas...
File media/filters/skcanvas_video_renderer.cc (right):

https://chromiumcodereview.appspot.com/11269017/diff/1/media/filters/skcanvas...
media/filters/skcanvas_video_renderer.cc:188: media::YUVType yuv_type =
(video_frame->format() == media::VideoFrame::YV12) ?
On 2012/10/25 00:43:07, Ami Fischman wrote:
> This thunk is wrong for the native texture case, where none of it is relevant.

> I think you want to move it into the IsEitherYV12OrYV16 block at l.212

Done.

https://chromiumcodereview.appspot.com/11269017/diff/1/media/filters/skcanvas...
media/filters/skcanvas_video_renderer.cc:208:
video_frame->data(media::VideoFrame::kVPlane) + uv_offset;
On 2012/10/25 00:43:07, Ami Fischman wrote:
> I think this thunk is correct (% needing to move per above), but it
*definitely*
> wants to be tested ;)

Done.

https://chromiumcodereview.appspot.com/11269017/diff/1/webkit/media/webvideof...
File webkit/media/webvideoframe_impl.cc (right):

https://chromiumcodereview.appspot.com/11269017/diff/1/webkit/media/webvideof...
webkit/media/webvideoframe_impl.cc:88: if (video_frame_.get())
On 2012/10/25 00:43:07, Ami Fischman wrote:
> flip the tests here and below to be consistent with the remaining methods
above.

Done.

[slavi, 2012-10-25 04:44:31]

https://chromiumcodereview.appspot.com/11269017/diff/5002/cc/video_layer_impl.cc
File cc/video_layer_impl.cc (right):

https://chromiumcodereview.appspot.com/11269017/diff/5002/cc/video_layer_impl...
cc/video_layer_impl.cc:174: WebKit::WebRect visibleRect =
m_frame->visibleRect();
I have a pending CL http://codereview.chromium.org/11274017/  that switches
video_layer_impl to use Chrome's concrete media::VideoFrame type.

That has a couple of positive effects. First, it doesn't do any virtual calls
through WebVideoFrame; it uses Chrome (gfx::Rect, etc) types instead of WebKit
(IntRect, etc) types which then get converted to Chrome types again; and you can
drop all your changes to webkit/ and third_party/WebKit.

I would suggest to pull those changes (as we spoke offline) from the other CL.

[slavi, 2012-10-25 05:38:27]

On 2012/10/25 04:44:31, slavi wrote:
> I have a pending CL http://codereview.chromium.org/11274017/  that switches
> video_layer_impl to use Chrome's concrete media::VideoFrame type.
> 
> That has a couple of positive effects. First, it doesn't do any virtual calls
> through WebVideoFrame; it uses Chrome (gfx::Rect, etc) types instead of WebKit
> (IntRect, etc) types which then get converted to Chrome types again; and you
can
> drop all your changes to webkit/ and third_party/WebKit.
> 
> I would suggest to pull those changes (as we spoke offline) from the other CL.

Just to clarify, you can drop your changes to third_party/WebKit and
webkit/media/webvideoframe_impl.h/cc since you can use WebVideoFrame just as
simple wrapper of media::VideoFrame which gets immediately unwrapped when it
gets back in Chrome land.

I think the single reason for the WebVideoFrame interface was that the
compositor used to live in WebKit. Now that it's in Chrome, ideally we can kill
WebVideoFrame or (not so ideally) make it an opaque empty interface.

If that seems like a too much effort, leave it as it is and I'll rebase my CL
once the rest of it is ready to land and adapt your changes to
video_layer_impl.cc to media::VideoFrame.

[Ami GONE FROM CHROMIUM, 2012-10-25 17:13:33]

LGTM % nits, and assuming this actually fixes the bugs you linked to :)

(note I slightly changed the layout of the BUG= lines you made in your CL
description, and added 140509)

http://codereview.chromium.org/11269017/diff/1/media/base/video_frame.cc
File media/base/video_frame.cc (right):

http://codereview.chromium.org/11269017/diff/1/media/base/video_frame.cc#newc...
media/base/video_frame.cc:60: natural_size.width() * natural_size.height() <=
limits::kMaxCanvas);
On 2012/10/25 01:53:54, sheu wrote:
> This kinda _is_ the helper static bool?
> 
> I mean I guess I could break out another function so that you could (yo dawg I
> herd u liek to) helper while you helper, but that seems excessive.

My point was that you were repeating the same set of 5 checks 3 different times.
 I don't feel strongly about this.
Actually the only thing I feel strongly about here as an ex-math-geek is that
Size & Rect should have IsDegenerate() methods.  

But wait!  They do, kinda!  {Rect,Size}::IsEmpty() can replace the non-(x,y)
>0's.

http://codereview.chromium.org/11269017/diff/5002/cc/layer_tree_host_impl_uni...
File cc/layer_tree_host_impl_unittest.cc (right):

http://codereview.chromium.org/11269017/diff/5002/cc/layer_tree_host_impl_uni...
cc/layer_tree_host_impl_unittest.cc:2542: virtual int stride(unsigned plane)
const { return 4; }
16?

http://codereview.chromium.org/11269017/diff/5002/cc/layer_tree_host_impl_uni...
cc/layer_tree_host_impl_unittest.cc:2546: virtual WebKit::WebRect visibleRect()
const { visibleRect; }
missing m_?
(how is this compiling? :))

http://codereview.chromium.org/11269017/diff/5002/cc/video_layer_impl.cc
File cc/video_layer_impl.cc (right):

http://codereview.chromium.org/11269017/diff/5002/cc/video_layer_impl.cc#newc...
cc/video_layer_impl.cc:126: // texture UVs.  For now, just expect (0, 0) offset.
IIUC you're saying none of the downstream creators of frames today generate
non-0,0 frames.  If that's what you're saying then this is fine.  If a frame
generator can produce non-0,0 based on encoded video, then this isn't fine :)

http://codereview.chromium.org/11269017/diff/5002/media/filters/skcanvas_vide...
File media/filters/skcanvas_video_renderer.cc (right):

http://codereview.chromium.org/11269017/diff/5002/media/filters/skcanvas_vide...
media/filters/skcanvas_video_renderer.cc:144: // TODO(hcfam): do rotation and
mirroring here.
You s/hclam/hcfam/'d incorrectly here.  Please revert.

[sheu, 2012-10-25 21:44:13]

One more draft.  This time also including unittests for
SkCanvasVideoRendererTest.

http://codereview.chromium.org/11269017/diff/1/media/base/video_frame.cc
File media/base/video_frame.cc (right):

http://codereview.chromium.org/11269017/diff/1/media/base/video_frame.cc#newc...
media/base/video_frame.cc:60: natural_size.width() * natural_size.height() <=
limits::kMaxCanvas);
Done, and more.

http://codereview.chromium.org/11269017/diff/5002/cc/layer_tree_host_impl_uni...
File cc/layer_tree_host_impl_unittest.cc (right):

http://codereview.chromium.org/11269017/diff/5002/cc/layer_tree_host_impl_uni...
cc/layer_tree_host_impl_unittest.cc:2542: virtual int stride(unsigned plane)
const { return 4; }
I guess for FormatNativeTexture that makes sense.  Done.

http://codereview.chromium.org/11269017/diff/5002/cc/layer_tree_host_impl_uni...
cc/layer_tree_host_impl_unittest.cc:2546: virtual WebKit::WebRect visibleRect()
const { visibleRect; }
Aha.  Looks like I have -build_tests -autotest in my USE flags for
chromeos-chrome :-)

http://codereview.chromium.org/11269017/diff/5002/cc/video_layer_impl.cc
File cc/video_layer_impl.cc (right):

http://codereview.chromium.org/11269017/diff/5002/cc/video_layer_impl.cc#newc...
cc/video_layer_impl.cc:126: // texture UVs.  For now, just expect (0, 0) offset.
Yep, everything generates 0,0 so far.  Updating comment some more.

http://codereview.chromium.org/11269017/diff/5002/cc/video_layer_impl.cc#newc...
cc/video_layer_impl.cc:174: WebKit::WebRect visibleRect =
m_frame->visibleRect();
I am lazy and didn't do this :-P.  The upstream WebKit change is in already.

http://codereview.chromium.org/11269017/diff/5002/media/filters/skcanvas_vide...
File media/filters/skcanvas_video_renderer.cc (right):

http://codereview.chromium.org/11269017/diff/5002/media/filters/skcanvas_vide...
media/filters/skcanvas_video_renderer.cc:144: // TODO(hcfam): do rotation and
mirroring here.
I am... not sure how that happened.  Done.

[jamesr, 2012-10-25 22:49:47]

lgtm

http://codereview.chromium.org/11269017/diff/18001/cc/layer_tree_host_impl_un...
File cc/layer_tree_host_impl_unittest.cc (right):

http://codereview.chromium.org/11269017/diff/18001/cc/layer_tree_host_impl_un...
cc/layer_tree_host_impl_unittest.cc:2538: FakeVideoFrame() : m_textureId(0),
m_visibleRect(0, 0, 4, 4) { memset(m_data, 0x80, sizeof(m_data)); }
One initialization per line. I would split this all out into:

FakeVideoFrame()
    : m_textureId(0)
    , m_visibleRect(...)
{
  memset(...)
}

http://codereview.chromium.org/11269017/diff/18001/cc/resource_provider.cc
File cc/resource_provider.cc (right):

http://codereview.chromium.org/11269017/diff/18001/cc/resource_provider.cc#ne...
cc/resource_provider.cc:293: DCHECK(resource->format == GL_RGBA);
DCHECK_EQ

[sheu, 2012-10-25 23:41:08]

Not good yet.

I ported my commit over to my non-ChromeOS Chromium checkout and built it, and
that looked fine.  Then I tried building the unittests...

I'll update in a bit.

[Ami GONE FROM CHROMIUM, 2012-10-26 02:46:52]

LGTM % you *believing* in the CL :)

http://codereview.chromium.org/11269017/diff/18001/media/filters/skcanvas_vid...
File media/filters/skcanvas_video_renderer_unittest.cc (right):

http://codereview.chromium.org/11269017/diff/18001/media/filters/skcanvas_vid...
media/filters/skcanvas_video_renderer_unittest.cc:113: //   Bl Bl R  R
s/Bl/B/g :)

[sheu, 2012-10-26 18:45:23]

Just by way of update -- it's almost all there; only remaining issue is test
failure on SkCanvasVideoRenderer, using fast-path painting.  Slow-path seems
fine.  Still debugging.

https://chromiumcodereview.appspot.com/11269017/diff/18001/media/filters/skca...
File media/filters/skcanvas_video_renderer_unittest.cc (right):

https://chromiumcodereview.appspot.com/11269017/diff/18001/media/filters/skca...
media/filters/skcanvas_video_renderer_unittest.cc:113: //   Bl Bl R  R
"Bl" for "black".  I realize that the colors in the actual static arrays aren't
correct -- these are fixed in the next revision.

[sheu, 2012-10-30 03:22:11]

Had to do a lot of unittest changing as well.

Good news: they all pass now!

[Ami GONE FROM CHROMIUM, 2012-10-30 05:51:43]

media/ still LGTM, though I'd appreciate it if Alpha could check/bless the YUV
specifics in
https://chromiumcodereview.appspot.com/11269017/diff/22001/media/filters/skca...

https://chromiumcodereview.appspot.com/11269017/diff/22001/media/filters/skca...
File media/filters/skcanvas_video_renderer_unittest.cc (right):

https://chromiumcodereview.appspot.com/11269017/diff/22001/media/filters/skca...
media/filters/skcanvas_video_renderer_unittest.cc:144: 0,   0,   0,   0,   0,  
0,   0,   0,  76,  76,  76,  76,  76,  76,  76,  76,
80-cols
(you can avoid wrapping by omitting one of the spaces in the last few columns,
where no 3-digit #s appear)

https://chromiumcodereview.appspot.com/11269017/diff/22001/media/filters/skca...
media/filters/skcanvas_video_renderer_unittest.cc:312: // FIXME sheu: commenting
out two checks due to http://crbug.com/158462.
s/FIXME sheu/TODO(sheu)/ - this is chromium, not webkit ;)

[sheu, 2012-10-30 09:16:49]

https://chromiumcodereview.appspot.com/11269017/diff/22001/media/filters/skca...
File media/filters/skcanvas_video_renderer_unittest.cc (right):

https://chromiumcodereview.appspot.com/11269017/diff/22001/media/filters/skca...
media/filters/skcanvas_video_renderer_unittest.cc:144: 0,   0,   0,   0,   0,  
0,   0,   0,  76,  76,  76,  76,  76,  76,  76,  76,
On 2012/10/30 05:51:43, Ami Fischman wrote:
> 80-cols
> (you can avoid wrapping by omitting one of the spaces in the last few columns,
> where no 3-digit #s appear)

Done.

https://chromiumcodereview.appspot.com/11269017/diff/22001/media/filters/skca...
media/filters/skcanvas_video_renderer_unittest.cc:312: // FIXME sheu: commenting
out two checks due to http://crbug.com/158462.
On 2012/10/30 05:51:43, Ami Fischman wrote:
> s/FIXME sheu/TODO(sheu)/ - this is chromium, not webkit ;)

Done.

[commit-bot: I haz the power, 2012-11-06 04:46:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/11269017/41001

[commit-bot: I haz the power, 2012-11-06 05:10:17]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-11-09 20:39:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/11269017/46014

[commit-bot: I haz the power, 2012-11-09 22:01:24]

Change committed as 166988