media::VideoFrame::WrapExternalSharedMemory

media::VideoFrame::WrapExternalSharedMemory

Add WrapExternalSharedMemory() for wrapping base::SharedMemory-backed buffers
in a media::VideoFrame

Also: add support for I420 in YUV handling paths for media::VideoFrame.

BUG=None
TEST=local build, run, unittests on desktop Linux

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

[sheu, 2013-08-08 05:43:09]

PTAL.

wjia@: content/browser/renderer_host/media/*
fischman@: media/base/*

[miu, 2013-08-08 18:03:31]

Please make sure TabCaptureApiTest.EndToEnd (in browser_tests) passes.

I'm wary about adding the I420 support in this change.  It's not equivalent to
YV12, so you'd need to make sure that both producers and consumers of
media::VideoFrame are handling the data correctly.

https://chromiumcodereview.appspot.com/22645005/diff/1/content/browser/render...
File content/browser/renderer_host/media/web_contents_video_capture_device.cc
(right):

https://chromiumcodereview.appspot.com/22645005/diff/1/content/browser/render...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:651: if
(output->format() != media::VideoFrame::I420) {
This shouldn't work: A very subtle detail, but the U and V planes are swapped
between YV12 and I420.  ;-)

[sheu, 2013-08-08 18:34:09]

Ran browser_tests, and found one DCHECK to update.

yuli@: PTAL

https://chromiumcodereview.appspot.com/22645005/diff/1/content/browser/render...
File content/browser/renderer_host/media/web_contents_video_capture_device.cc
(right):

https://chromiumcodereview.appspot.com/22645005/diff/1/content/browser/render...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:651: if
(output->format() != media::VideoFrame::I420) {
On 2013/08/08 18:03:31, Yuri wrote:
> This shouldn't work: A very subtle detail, but the U and V planes are swapped
> between YV12 and I420.  ;-)

They're swapped, but we access the frame's data through frame->data(PLANE), so
the planar data is being dumped into the right locations

[sheu, 2013-08-08 18:34:29]

miu@, rather

[danakj, 2013-08-08 18:34:52]

On 2013/08/08 18:34:09, sheu wrote:
> Ran browser_tests, and found one DCHECK to update.
> 
> yuli@: PTAL
> 
>
https://chromiumcodereview.appspot.com/22645005/diff/1/content/browser/render...
> File content/browser/renderer_host/media/web_contents_video_capture_device.cc
> (right):
> 
>
https://chromiumcodereview.appspot.com/22645005/diff/1/content/browser/render...
> content/browser/renderer_host/media/web_contents_video_capture_device.cc:651:
if
> (output->format() != media::VideoFrame::I420) {
> On 2013/08/08 18:03:31, Yuri wrote:
> > This shouldn't work: A very subtle detail, but the U and V planes are
swapped
> > between YV12 and I420.  ;-)
> 
> They're swapped, but we access the frame's data through frame->data(PLANE), so
> the planar data is being dumped into the right locations

If you're planning to display a new format, the VideoResourceUpdater in cc/ will
need to be changed.

[sheu, 2013-08-08 18:38:08]

danakj: there's no addition to the display formats.  It's for transport only at
the moment.

[miu, 2013-08-08 19:33:56]

lgtm

Good stuff!  I assume this will help us reduce memcpy's as data is passed
between processes?

https://chromiumcodereview.appspot.com/22645005/diff/6001/content/browser/ren...
File content/browser/renderer_host/media/web_contents_video_capture_device.cc
(right):

https://chromiumcodereview.appspot.com/22645005/diff/6001/content/browser/ren...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:651: if
(output->format() != media::VideoFrame::I420) {
nit: This could be:

  if (output->format() != media::VideoFrame::YV12 &&
      output->format() != media::VideoFrame::I420) {
    NOTREACHED();
    return;
  }

[Ami GONE FROM CHROMIUM, 2013-08-08 19:34:28]

https://chromiumcodereview.appspot.com/22645005/diff/6001/media/base/video_fr...
File media/base/video_frame.cc (right):

https://chromiumcodereview.appspot.com/22645005/diff/6001/media/base/video_fr...
media/base/video_frame.cc:121: scoped_refptr<VideoFrame>
VideoFrame::WrapExternalSharedMemory(
What is the benefit of this new function over the old WrapExternalYuvData?
The only diff I can see is that this only takes a single data ptr and computes
planar offsets from that (as opposed to taking plane pointers) but that seems
unuseful since it means that this function needs to do math to find offsets and
hope that the result fits in the SHM that data is mapping.  Would be better for
the caller to do the math, since the caller is better placed to know the limits
of the SHM, no?

[sheu, 2013-08-08 19:43:08]

https://chromiumcodereview.appspot.com/22645005/diff/6001/content/browser/ren...
File content/browser/renderer_host/media/web_contents_video_capture_device.cc
(right):

https://chromiumcodereview.appspot.com/22645005/diff/6001/content/browser/ren...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:651: if
(output->format() != media::VideoFrame::I420) {
On 2013/08/08 19:33:56, Yuri wrote:
> nit: This could be:
> 
>   if (output->format() != media::VideoFrame::YV12 &&
>       output->format() != media::VideoFrame::I420) {
>     NOTREACHED();
>     return;
>   }

We're expecting I420 now; I'd leave it as is.

https://chromiumcodereview.appspot.com/22645005/diff/6001/media/base/video_fr...
File media/base/video_frame.cc (right):

https://chromiumcodereview.appspot.com/22645005/diff/6001/media/base/video_fr...
media/base/video_frame.cc:121: scoped_refptr<VideoFrame>
VideoFrame::WrapExternalSharedMemory(
On 2013/08/08 19:34:28, Ami Fischman wrote:
> What is the benefit of this new function over the old WrapExternalYuvData?
> The only diff I can see is that this only takes a single data ptr and computes
> planar offsets from that (as opposed to taking plane pointers) but that seems
> unuseful since it means that this function needs to do math to find offsets
and
> hope that the result fits in the SHM that data is mapping.  Would be better
for
> the caller to do the math, since the caller is better placed to know the
limits
> of the SHM, no?

This code features in the WebRTC VEA changelist when we're receiving SHM in
GpuVEA and constructing a media::VideoFrame from it.  I noticed that for the
capturer implementation I was about to copy-paste it again, so I figured it
would be better avoid copypasta and put it in once place.

So yeah there's only one caller for now, but there will soon be multiple.

[Ami GONE FROM CHROMIUM, 2013-08-08 19:49:38]

https://chromiumcodereview.appspot.com/22645005/diff/6001/media/base/video_fr...
File media/base/video_frame.cc (right):

https://chromiumcodereview.appspot.com/22645005/diff/6001/media/base/video_fr...
media/base/video_frame.cc:121: scoped_refptr<VideoFrame>
VideoFrame::WrapExternalSharedMemory(
On 2013/08/08 19:43:08, sheu wrote:
> On 2013/08/08 19:34:28, Ami Fischman wrote:
> > What is the benefit of this new function over the old WrapExternalYuvData?
> > The only diff I can see is that this only takes a single data ptr and
computes
> > planar offsets from that (as opposed to taking plane pointers) but that
seems
> > unuseful since it means that this function needs to do math to find offsets
> and
> > hope that the result fits in the SHM that data is mapping.  Would be better
> for
> > the caller to do the math, since the caller is better placed to know the
> limits
> > of the SHM, no?
> 
> This code features in the WebRTC VEA changelist when we're receiving SHM in
> GpuVEA and constructing a media::VideoFrame from it.  I noticed that for the
> capturer implementation I was about to copy-paste it again, so I figured it
> would be better avoid copypasta and put it in once place.
> 
> So yeah there's only one caller for now, but there will soon be multiple.


Then can WEYD be deleted and VCBP use this new method instead?
(AFAICT that's the only caller of WEYD)

[sheu, 2013-08-08 19:55:38]

https://chromiumcodereview.appspot.com/22645005/diff/6001/media/base/video_fr...
File media/base/video_frame.cc (right):

https://chromiumcodereview.appspot.com/22645005/diff/6001/media/base/video_fr...
media/base/video_frame.cc:121: scoped_refptr<VideoFrame>
VideoFrame::WrapExternalSharedMemory(
On 2013/08/08 19:49:38, Ami Fischman wrote:
> On 2013/08/08 19:43:08, sheu wrote:
> > On 2013/08/08 19:34:28, Ami Fischman wrote:
> > > What is the benefit of this new function over the old WrapExternalYuvData?
> > > The only diff I can see is that this only takes a single data ptr and
> computes
> > > planar offsets from that (as opposed to taking plane pointers) but that
> seems
> > > unuseful since it means that this function needs to do math to find
offsets
> > and
> > > hope that the result fits in the SHM that data is mapping.  Would be
better
> > for
> > > the caller to do the math, since the caller is better placed to know the
> > limits
> > > of the SHM, no?
> > 
> > This code features in the WebRTC VEA changelist when we're receiving SHM in
> > GpuVEA and constructing a media::VideoFrame from it.  I noticed that for the
> > capturer implementation I was about to copy-paste it again, so I figured it
> > would be better avoid copypasta and put it in once place.
> > 
> > So yeah there's only one caller for now, but there will soon be multiple.
> 
> 
> Then can WEYD be deleted and VCBP use this new method instead?
> (AFAICT that's the only caller of WEYD)

I already switched VCBP over to this function in this CL.

Unfortunately there's another path that uses WEYD, and it's on the PPAPI path. 
There is a way to switch it over too, but that's going to involve a nontrivial
respec of some (thankfully internal) PPAPI bits, which I could do, but which
would require the PPAPI guys to sign off on, which would hold things up.

Also the unittest uses WEYD, but that's less of an issue.

So one thing at a time.

[Ami GONE FROM CHROMIUM, 2013-08-08 20:05:28]

lgtm

https://chromiumcodereview.appspot.com/22645005/diff/6001/media/base/video_fr...
File media/base/video_frame.h (right):

https://chromiumcodereview.appspot.com/22645005/diff/6001/media/base/video_fr...
media/base/video_frame.h:157: // is destroyed |no_longer_needed_cb.Run()| will
be called.
TODO+crbug for the cleanup please.

[sheu, 2013-08-08 20:15:38]

OWNERS checkoffs from:

yzshen@
danakj@

PTAL.

https://chromiumcodereview.appspot.com/22645005/diff/6001/media/base/video_fr...
File media/base/video_frame.h (right):

https://chromiumcodereview.appspot.com/22645005/diff/6001/media/base/video_fr...
media/base/video_frame.h:157: // is destroyed |no_longer_needed_cb.Run()| will
be called.
On 2013/08/08 20:05:28, Ami Fischman wrote:
> TODO+crbug for the cleanup please.

Done.

[danakj, 2013-08-08 22:05:24]

cc lgtm

[yzshen1, 2013-08-08 22:23:03]

LGTM for .*pepper.*

[commit-bot: I haz the power, 2013-08-08 22:29:56]

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

[commit-bot: I haz the power, 2013-08-09 00:13:01]

Retried try job too often on linux_chromeos for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-08-09 00:19:30]

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

[commit-bot: I haz the power, 2013-08-09 08:04:39]

Change committed as 216641