Handle Alpha channel in Canvas capture

Handle Alpha channel in Canvas capture

This CL changes the output frame format in canvas capture from
I420 to YV12A so that alpha channel is preserved. The change
discussion is here [0].

As a result, each MediaStreamSink would be responsible for
handling YV12A frames. For that purpose
VideoUtil::WrapAsI420VideoFrame() is added to each appropriate
sink.

[0] https://github.com/w3c/mediacapture-fromelement/issues/30
[1] https://code.google.com/p/chromium/codesearch#chromium/src/content/public/renderer/media_stream_sink.h&l=18&ct=xref_jump_to_def&cl=GROK&gsn=MediaStreamSink
BUG=524218
TEST= https://cdn.rawgit.com/uysalere/js-demos/master/canvascapture3.html
TBR=bbudge@chromium.org

Committed: https://crrev.com/ec1e7ff3b75787d3290ba5a899db8ef4ca79d302
Cr-Commit-Position: refs/heads/master@{#379932}

[emircan, 2016-02-25 20:00:47]

Description was changed from

==========
handle alpha.

BUG=
==========

to

==========
Handle Alpha channel in Canvas capture

This CL changes the output frame format in canvas capture from
I420 to YV12A so that alpha channel is preserved. The change
discussion is here [0].

As a result, each MediaStreamSink would be responsible for
handling YV12A frames. For that purpose 
VideoFrame::DropYV12AAlphaChannel() is added to each appropriate
sink.

[0] https://github.com/w3c/mediacapture-fromelement/issues/30
[1]
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/ren...
BUG=524218
TEST= https://cdn.rawgit.com/uysalere/js-demos/master/canvascapture3.html
==========

[emircan, 2016-02-25 20:00:47]

emircan@chromium.org changed reviewers:
+ mcasas@chromium.org, philipj@opera.com

[emircan, 2016-02-25 20:01:20]

PTAL.

[mcasas, 2016-02-25 22:06:26]

https://codereview.chromium.org/1737253002/diff/1/content/renderer/media/canv...
File content/renderer/media/canvas_capture_handler.cc (right):

https://codereview.chromium.org/1737253002/diff/1/content/renderer/media/canv...
content/renderer/media/canvas_capture_handler.cc:201:
video_frame->data(media::VideoFrame::kAPlane)[p] = temp_data_[p * 4 + 3];
Hmm it looks like this deinterlaced could lower 
the performance significantly. I'd love to use 
libyuv::CopyPlane() [1]  but here it doesn't 
match due to the 4*.. + 3 extra calculation. I 
wonder if we could go back to |image| and use 
its readYUV8Planes() [2]? Ah but it doesn't
support Alpha either! :(

Could we at least skip this process overall if
we see somehow that the alpha plane is not
used? We don't expect that every single canvas
would make use of it, right?

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libyuv...
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/i...

https://codereview.chromium.org/1737253002/diff/1/content/renderer/media/vide...
File content/renderer/media/video_track_recorder.cc (right):

https://codereview.chromium.org/1737253002/diff/1/content/renderer/media/vide...
content/renderer/media/video_track_recorder.cc:195: if (!(frame->IsMappable() &&
Why add this here now? (Same elsewhere).

https://codereview.chromium.org/1737253002/diff/1/content/renderer/media/vide...
content/renderer/media/video_track_recorder.cc:200: }
Judgment call: turn this into a DCHECK() and
let it continue? AFAIK there's nothing but I420
frames on the renderer MSs before this CL.

https://codereview.chromium.org/1737253002/diff/1/content/renderer/media/webm...
File content/renderer/media/webmediaplayer_ms.cc (right):

https://codereview.chromium.org/1737253002/diff/1/content/renderer/media/webm...
content/renderer/media/webmediaplayer_ms.cc:450:
DCHECK(thread_checker_.CalledOnValidThread());
nit: DCHECK(I420 or YV12A) ?

https://codereview.chromium.org/1737253002/diff/1/content/renderer/media/webm...
content/renderer/media/webmediaplayer_ms.cc:473:
video_weblayer_->layer()->SetContentsOpaque(false);
SetContentsOpaque(frame->format() != media::PIXEL_FORMAT_YV12A) ?

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

https://codereview.chromium.org/1737253002/diff/1/media/base/video_frame.cc#n...
media/base/video_frame.cc:867: LOG(DFATAL) << "Only STORAGE_OWNED_MEMORY format
supported.";
What about ShMeM? I'd say either 
consider all cases (since alpha can easily 
be nullified for e.g. GMBs etc) or 
just a DCHECK for the supported 
storage types.

[Pawel Osciak, 2016-02-26 00:32:50]

posciak@chromium.org changed reviewers:
+ posciak@chromium.org

[Pawel Osciak, 2016-02-26 00:32:51]

https://codereview.chromium.org/1737253002/diff/1/media/base/video_frame.h
File media/base/video_frame.h (right):

https://codereview.chromium.org/1737253002/diff/1/media/base/video_frame.h#ne...
media/base/video_frame.h:382: // Converts a frame with YV12A format into I420 by
dropping alpha channel.
Personally, I'm wondering if this kind of a method should be in VideoFrame
itself, not somewhere else? VF is quite complicated already...

[emircan, 2016-02-26 01:33:08]

https://codereview.chromium.org/1737253002/diff/1/content/renderer/media/canv...
File content/renderer/media/canvas_capture_handler.cc (right):

https://codereview.chromium.org/1737253002/diff/1/content/renderer/media/canv...
content/renderer/media/canvas_capture_handler.cc:201:
video_frame->data(media::VideoFrame::kAPlane)[p] = temp_data_[p * 4 + 3];
On 2016/02/25 22:06:25, mcasas wrote:
> Hmm it looks like this deinterlaced could lower 
> the performance significantly. I'd love to use 
> libyuv::CopyPlane() [1]  but here it doesn't 
> match due to the 4*.. + 3 extra calculation. I 
> wonder if we could go back to |image| and use 
> its readYUV8Planes() [2]? Ah but it doesn't
> support Alpha either! :(
> 
> Could we at least skip this process overall if
> we see somehow that the alpha plane is not
> used? We don't expect that every single canvas
> would make use of it, right?
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libyuv...
> [2]
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/i...

Unfortunately, there isn't a libyuv method I can directly use for now. I talked
with owners and they agreed to work on one . You can see updates on
https://code.google.com/p/libyuv/issues/detail?id=572. readYUV8Planes() also
doesn't have alpha, and works much slower than libyuv :(

Regarding alpha plane check, I started using SkImage::isOpaque().

https://codereview.chromium.org/1737253002/diff/1/content/renderer/media/vide...
File content/renderer/media/video_track_recorder.cc (right):

https://codereview.chromium.org/1737253002/diff/1/content/renderer/media/vide...
content/renderer/media/video_track_recorder.cc:195: if (!(frame->IsMappable() &&
On 2016/02/25 22:06:25, mcasas wrote:
> Why add this here now? (Same elsewhere).

Removing IsMappable() checks.

https://codereview.chromium.org/1737253002/diff/1/content/renderer/media/vide...
content/renderer/media/video_track_recorder.cc:200: }
On 2016/02/25 22:06:25, mcasas wrote:
> Judgment call: turn this into a DCHECK() and
> let it continue? AFAIK there's nothing but I420
> frames on the renderer MSs before this CL.

I actually started with DCHECKs but then found [0], so I decided to follow the
pattern. I know that DCHECKs would be better for performance, but there might be
cases out in the wild that isn't covered by the current tests. It is a hard
judgment call. WDYT?

[0]
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

https://codereview.chromium.org/1737253002/diff/1/content/renderer/media/webm...
File content/renderer/media/webmediaplayer_ms.cc (right):

https://codereview.chromium.org/1737253002/diff/1/content/renderer/media/webm...
content/renderer/media/webmediaplayer_ms.cc:450:
DCHECK(thread_checker_.CalledOnValidThread());
On 2016/02/25 22:06:25, mcasas wrote:
> nit: DCHECK(I420 or YV12A) ?

I don't think it is necessary. VideoResourceUpdater::CreateForSoftwarePlanes
shows that we support many other formats, so it wouldn't hurt to send them down
the line as well.

[0]
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/video...

https://codereview.chromium.org/1737253002/diff/1/content/renderer/media/webm...
content/renderer/media/webmediaplayer_ms.cc:473:
video_weblayer_->layer()->SetContentsOpaque(false);
On 2016/02/25 22:06:25, mcasas wrote:
> SetContentsOpaque(frame->format() != media::PIXEL_FORMAT_YV12A) ?
> 

This value is set once. Since we started switching between YV12A and I420 after
the last changes, we can no longer depend on first frame representing the whole.

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

https://codereview.chromium.org/1737253002/diff/1/media/base/video_frame.cc#n...
media/base/video_frame.cc:867: LOG(DFATAL) << "Only STORAGE_OWNED_MEMORY format
supported.";
On 2016/02/25 22:06:25, mcasas wrote:
> What about ShMeM? I'd say either 
> consider all cases (since alpha can easily 
> be nullified for e.g. GMBs etc) or 
> just a DCHECK for the supported 
> storage types.
Adding DCHECKs for supported types.

https://codereview.chromium.org/1737253002/diff/1/media/base/video_frame.h
File media/base/video_frame.h (right):

https://codereview.chromium.org/1737253002/diff/1/media/base/video_frame.h#ne...
media/base/video_frame.h:382: // Converts a frame with YV12A format into I420 by
dropping alpha channel.
On 2016/02/26 00:32:51, Pawel Osciak wrote:
> Personally, I'm wondering if this kind of a method should be in VideoFrame
> itself, not somewhere else? VF is quite complicated already...

I see your point about VF being too complicated again. However, I am modifying
private class members in this function and it belongs here more than a helper
class. WDYT?

[Pawel Osciak, 2016-02-26 01:51:25]

https://codereview.chromium.org/1737253002/diff/1/media/base/video_frame.h
File media/base/video_frame.h (right):

https://codereview.chromium.org/1737253002/diff/1/media/base/video_frame.h#ne...
media/base/video_frame.h:382: // Converts a frame with YV12A format into I420 by
dropping alpha channel.
On 2016/02/26 01:33:08, emircan wrote:
> On 2016/02/26 00:32:51, Pawel Osciak wrote:
> > Personally, I'm wondering if this kind of a method should be in VideoFrame
> > itself, not somewhere else? VF is quite complicated already...
> 
> I see your point about VF being too complicated again. However, I am modifying
> private class members in this function and it belongs here more than a helper
> class. WDYT?

Honestly, I would really prefer not doing it this way. I understand this will
somewhat work, assuming only for OWNED_MEMORY *and* due to the fact how we
allocate (just one buffer) *and* how AlignedAlloc/Free work.

But this is very scary in case things change. And this is very, very non-obvious
to anyone changing this later on, for example, it's not obvious why this only
works for OWNED_MEMORY and how, if someone decides to extend this later; it's
not obvious that we may only be using one-buffer allocation and malloc/free that
stores size by itself, not using the sizes in VF, etc.

There is also the code complexity which I believe we should try not to introduce
to VF if we can help it. Could we for example use realloc outside of this class
and re-Wrap() the frame as different format?

[emircan, 2016-02-27 02:51:30]

Patchset #2 (id:20001) has been deleted

[emircan, 2016-02-27 02:54:39]

Patchset #2 (id:40001) has been deleted

[emircan, 2016-02-29 20:57:39]

On 2016/02/26 01:51:25, Pawel Osciak wrote:
> https://codereview.chromium.org/1737253002/diff/1/media/base/video_frame.h
> File media/base/video_frame.h (right):
> 
>
https://codereview.chromium.org/1737253002/diff/1/media/base/video_frame.h#ne...
> media/base/video_frame.h:382: // Converts a frame with YV12A format into I420
by
> dropping alpha channel.
> On 2016/02/26 01:33:08, emircan wrote:
> > On 2016/02/26 00:32:51, Pawel Osciak wrote:
> > > Personally, I'm wondering if this kind of a method should be in VideoFrame
> > > itself, not somewhere else? VF is quite complicated already...
> > 
> > I see your point about VF being too complicated again. However, I am
modifying
> > private class members in this function and it belongs here more than a
helper
> > class. WDYT?
> 
> Honestly, I would really prefer not doing it this way. I understand this will
> somewhat work, assuming only for OWNED_MEMORY *and* due to the fact how we
> allocate (just one buffer) *and* how AlignedAlloc/Free work.
> 
> But this is very scary in case things change. And this is very, very
non-obvious
> to anyone changing this later on, for example, it's not obvious why this only
> works for OWNED_MEMORY and how, if someone decides to extend this later; it's
> not obvious that we may only be using one-buffer allocation and malloc/free
that
> stores size by itself, not using the sizes in VF, etc.
> 
> There is also the code complexity which I believe we should try not to
introduce
> to VF if we can help it. Could we for example use realloc outside of this
class
> and re-Wrap() the frame as different format?

I modified  VideoFrame::WrapVideoFrame function to include a target pixel
format, and moved this new helper to video_utils. I will refrain from using
realloc in case there are multiple sink accessing the same video frame. PTAL.

[mcasas, 2016-02-29 23:48:01]

https://codereview.chromium.org/1737253002/diff/60001/content/renderer/media/...
File content/renderer/media/canvas_capture_handler.cc (right):

https://codereview.chromium.org/1737253002/diff/60001/content/renderer/media/...
content/renderer/media/canvas_capture_handler.cc:185: const bool isOpaque =
image->isOpaque();
Move to l.190.

https://codereview.chromium.org/1737253002/diff/60001/content/renderer/media/...
File content/renderer/media/media_stream_video_renderer_sink_unittest.cc
(right):

https://codereview.chromium.org/1737253002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_video_renderer_sink_unittest.cc:187: const
gfx::Size size(10, 10);
mini nit: s/size/kSize/ ?

https://codereview.chromium.org/1737253002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_video_renderer_sink_unittest.cc:196: 
nit: remove l.196.

https://codereview.chromium.org/1737253002/diff/60001/media/base/video_frame.cc
File media/base/video_frame.cc (right):

https://codereview.chromium.org/1737253002/diff/60001/media/base/video_frame....
media/base/video_frame.cc:219: }
Hmm actually I'd say either they are the same
format, or are immediately compatible, so the
comparison should be sth reflecting that
either |source_format| or |target_format| is
PIXEL_FORMAT_{YV12,I420,YV12A}, --> OK 

otherwise we might skip previously supported 
cases?

https://codereview.chromium.org/1737253002/diff/60001/media/base/video_frame....
media/base/video_frame.cc:527: LOG(DFATAL) << __FUNCTION__ << " Invalid
formats."
s/formats./format conversion: /

https://codereview.chromium.org/1737253002/diff/60001/media/base/video_frame....
media/base/video_frame.cc:548: }
nit: Maybe we should copy all VideoFrameMetadata::Key ?

I'm ok with crbug.com too.

https://codereview.chromium.org/1737253002/diff/60001/media/base/video_util.cc
File media/base/video_util.cc (right):

https://codereview.chromium.org/1737253002/diff/60001/media/base/video_util.c...
media/base/video_util.cc:20: // The reference to |frame| is kept in the closure
that calls this method.
nit: This second sentence doesn't make sense :)

https://codereview.chromium.org/1737253002/diff/60001/media/base/video_util.c...
media/base/video_util.cc:21: void ReleaseOriginalFrame(const
scoped_refptr<media::VideoFrame>& frame) {}
Can we use base::DeletePointer? [1]

Also, no empty line 22.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/base/bind_helpers....

https://codereview.chromium.org/1737253002/diff/60001/media/base/video_util.h
File media/base/video_util.h (right):

https://codereview.chromium.org/1737253002/diff/60001/media/base/video_util.h...
media/base/video_util.h:97: MEDIA_EXPORT scoped_refptr<VideoFrame>
DropYV12AAlphaChannel(
I was surprised that it returned a scoped_refptr<VideoFrame>
when the name seems to imply that it drops the A channel of
the passed frame. Please update name and comment to 
reflect that it wraps a given frame into a non-alpha frame.
Hmmm what about AsOpaque() ... ?

[emircan, 2016-03-01 20:07:55]

https://codereview.chromium.org/1737253002/diff/60001/content/renderer/media/...
File content/renderer/media/canvas_capture_handler.cc (right):

https://codereview.chromium.org/1737253002/diff/60001/content/renderer/media/...
content/renderer/media/canvas_capture_handler.cc:185: const bool isOpaque =
image->isOpaque();
On 2016/02/29 23:48:00, mcasas wrote:
> Move to l.190.

Done.

https://codereview.chromium.org/1737253002/diff/60001/content/renderer/media/...
File content/renderer/media/media_stream_video_renderer_sink_unittest.cc
(right):

https://codereview.chromium.org/1737253002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_video_renderer_sink_unittest.cc:187: const
gfx::Size size(10, 10);
On 2016/02/29 23:48:01, mcasas wrote:
> mini nit: s/size/kSize/ ?

Done.

https://codereview.chromium.org/1737253002/diff/60001/content/renderer/media/...
content/renderer/media/media_stream_video_renderer_sink_unittest.cc:196: 
On 2016/02/29 23:48:00, mcasas wrote:
> nit: remove l.196.

Done.

https://codereview.chromium.org/1737253002/diff/60001/media/base/video_frame.cc
File media/base/video_frame.cc (right):

https://codereview.chromium.org/1737253002/diff/60001/media/base/video_frame....
media/base/video_frame.cc:219: }
On 2016/02/29 23:48:01, mcasas wrote:
> Hmm actually I'd say either they are the same
> format, or are immediately compatible, so the
> comparison should be sth reflecting that
> either |source_format| or |target_format| is
> PIXEL_FORMAT_{YV12,I420,YV12A}, --> OK 
> 
> otherwise we might skip previously supported 
> cases?

l.206 covers the previously supported cases. 

For the new cases, I wanted to only expose PIXEL_FORMAT_YV12A->PIXEL_FORMAT_I420
because only that would be used for now. Additional cases may be added and
tested as use cases come. WDYT?

https://codereview.chromium.org/1737253002/diff/60001/media/base/video_frame....
media/base/video_frame.cc:527: LOG(DFATAL) << __FUNCTION__ << " Invalid
formats."
On 2016/02/29 23:48:01, mcasas wrote:
> s/formats./format conversion: /

Done.

https://codereview.chromium.org/1737253002/diff/60001/media/base/video_frame....
media/base/video_frame.cc:548: }
On 2016/02/29 23:48:01, mcasas wrote:
> nit: Maybe we should copy all VideoFrameMetadata::Key ?
> 
> I'm ok with http://crbug.com too.

Done.

https://codereview.chromium.org/1737253002/diff/60001/media/base/video_util.cc
File media/base/video_util.cc (right):

https://codereview.chromium.org/1737253002/diff/60001/media/base/video_util.c...
media/base/video_util.cc:20: // The reference to |frame| is kept in the closure
that calls this method.
On 2016/02/29 23:48:01, mcasas wrote:
> nit: This second sentence doesn't make sense :)

Done.

https://codereview.chromium.org/1737253002/diff/60001/media/base/video_util.c...
media/base/video_util.cc:21: void ReleaseOriginalFrame(const
scoped_refptr<media::VideoFrame>& frame) {}
On 2016/02/29 23:48:01, mcasas wrote:
> Can we use base::DeletePointer? [1]
> 
> Also, no empty line 22.
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/bind_helpers....

DeletePointer doesn't support ref_ptr as it only expects a naked ptr. Also, the
space is for the namespace.

https://codereview.chromium.org/1737253002/diff/60001/media/base/video_util.h
File media/base/video_util.h (right):

https://codereview.chromium.org/1737253002/diff/60001/media/base/video_util.h...
media/base/video_util.h:97: MEDIA_EXPORT scoped_refptr<VideoFrame>
DropYV12AAlphaChannel(
On 2016/02/29 23:48:01, mcasas wrote:
> I was surprised that it returned a scoped_refptr<VideoFrame>
> when the name seems to imply that it drops the A channel of
> the passed frame. Please update name and comment to 
> reflect that it wraps a given frame into a non-alpha frame.
> Hmmm what about AsOpaque() ... ?

I see your point. WrapAsOpaque() might also imply RGBA->RGB. I will go with
WrapAsI420VideoFrame(). WDYT?

[emircan, 2016-03-02 21:53:28]

Friendly ping. Can you PTAL?

[mcasas, 2016-03-02 22:01:10]

LGTM % comments.

https://codereview.chromium.org/1737253002/diff/60001/media/base/video_frame.cc
File media/base/video_frame.cc (right):

https://codereview.chromium.org/1737253002/diff/60001/media/base/video_frame....
media/base/video_frame.cc:219: }
On 2016/03/01 20:07:55, emircan wrote:
> On 2016/02/29 23:48:01, mcasas wrote:
> > Hmm actually I'd say either they are the same
> > format, or are immediately compatible, so the
> > comparison should be sth reflecting that
> > either |source_format| or |target_format| is
> > PIXEL_FORMAT_{YV12,I420,YV12A}, --> OK 
> > 
> > otherwise we might skip previously supported 
> > cases?
> 
> l.206 covers the previously supported cases. 
> 
> For the new cases, I wanted to only expose
PIXEL_FORMAT_YV12A->PIXEL_FORMAT_I420
> because only that would be used for now. Additional cases may be added and
> tested as use cases come. WDYT?

SG。I'd say just add a comment saying that
nothing prevents trivially adding other 
triplanar-to-triplanar conversions, and change 
this code to

  return source_format == PIXEL_FORMAT_YV12A &&
         target_format == PIXEL_FORMAT_I420;

https://codereview.chromium.org/1737253002/diff/80001/media/base/video_frame.cc
File media/base/video_frame.cc (right):

https://codereview.chromium.org/1737253002/diff/80001/media/base/video_frame....
media/base/video_frame.cc:527: LOG(DFATAL) << __FUNCTION__ << " Invalid format
conversions."
micro-nit: s/conversions./conversion: /

[emircan, 2016-03-03 22:02:16]

emircan@chromium.org changed reviewers:
- philipj@opera.com

[emircan, 2016-03-03 22:05:58]

https://codereview.chromium.org/1737253002/diff/60001/media/base/video_frame.cc
File media/base/video_frame.cc (right):

https://codereview.chromium.org/1737253002/diff/60001/media/base/video_frame....
media/base/video_frame.cc:219: }
On 2016/03/02 22:01:10, mcasas wrote:
> On 2016/03/01 20:07:55, emircan wrote:
> > On 2016/02/29 23:48:01, mcasas wrote:
> > > Hmm actually I'd say either they are the same
> > > format, or are immediately compatible, so the
> > > comparison should be sth reflecting that
> > > either |source_format| or |target_format| is
> > > PIXEL_FORMAT_{YV12,I420,YV12A}, --> OK 
> > > 
> > > otherwise we might skip previously supported 
> > > cases?
> > 
> > l.206 covers the previously supported cases. 
> > 
> > For the new cases, I wanted to only expose
> PIXEL_FORMAT_YV12A->PIXEL_FORMAT_I420
> > because only that would be used for now. Additional cases may be added and
> > tested as use cases come. WDYT?
> 
> SG。I'd say just add a comment saying that
> nothing prevents trivially adding other 
> triplanar-to-triplanar conversions, and change 
> this code to
> 
>   return source_format == PIXEL_FORMAT_YV12A &&
>          target_format == PIXEL_FORMAT_I420;

Done.

https://codereview.chromium.org/1737253002/diff/80001/media/base/video_frame.cc
File media/base/video_frame.cc (right):

https://codereview.chromium.org/1737253002/diff/80001/media/base/video_frame....
media/base/video_frame.cc:527: LOG(DFATAL) << __FUNCTION__ << " Invalid format
conversions."
On 2016/03/02 22:01:10, mcasas wrote:
> micro-nit: s/conversions./conversion: /

Done.

[emircan, 2016-03-03 22:08:15]

Description was changed from

==========
Handle Alpha channel in Canvas capture

This CL changes the output frame format in canvas capture from
I420 to YV12A so that alpha channel is preserved. The change
discussion is here [0].

As a result, each MediaStreamSink would be responsible for
handling YV12A frames. For that purpose 
VideoFrame::DropYV12AAlphaChannel() is added to each appropriate
sink.

[0] https://github.com/w3c/mediacapture-fromelement/issues/30
[1]
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/ren...
BUG=524218
TEST= https://cdn.rawgit.com/uysalere/js-demos/master/canvascapture3.html
==========

to

==========
Handle Alpha channel in Canvas capture

This CL changes the output frame format in canvas capture from
I420 to YV12A so that alpha channel is preserved. The change
discussion is here [0].

As a result, each MediaStreamSink would be responsible for
handling YV12A frames. For that purpose 
VideoUtil::WrapAsI420VideoFrame() is added to each appropriate
sink.

[0] https://github.com/w3c/mediacapture-fromelement/issues/30
[1]
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/ren...
BUG=524218
TEST= https://cdn.rawgit.com/uysalere/js-demos/master/canvascapture3.html
==========

[emircan, 2016-03-03 22:10:49]

emircan@chromium.org changed reviewers:
+ dalecurtis@chromium.org, sievers@chromium.org

[emircan, 2016-03-03 22:10:49]

I am adding OWNERS for review.

dalecurtis@, can you review media/*?
content/renderer/pepper/pepper_video_source_host.cc

[emircan, 2016-03-03 22:11:26]

emircan@chromium.org changed reviewers:
- sievers@chromium.org

[DaleCurtis, 2016-03-07 20:43:13]

lgtm

[emircan, 2016-03-07 20:48:22]

emircan@chromium.org changed reviewers:
+ esprehn@chromium.org

[emircan, 2016-03-07 20:48:25]

Thanks. 

esprehn@, can you review content/renderer/pepper/* as OWNERS?

[esprehn, 2016-03-07 21:37:23]

On 2016/03/07 at 20:48:25, emircan wrote:
> Thanks. 
> 
> esprehn@, can you review content/renderer/pepper/* as OWNERS?

That's not a part of content/ I'm familiar with, I'd suggest asking someone who
understands what pepper is doing there. ex. someone from
content/renderer/pepper/OWNERS

[emircan, 2016-03-07 21:59:19]

emircan@chromium.org changed reviewers:
+ raymes@chromium.org

[emircan, 2016-03-07 21:59:20]

Thanks. 

raymes@, can you review content/renderer/pepper/* as OWNERS?

[raymes, 2016-03-07 23:00:00]

raymes@chromium.org changed reviewers:
+ bbudge@chromium.org, penghuang@chromium.org

[raymes, 2016-03-07 23:00:01]

I'm not so familiar with this pepper code. +bbudge or penghuang

Or if Dale has had a look at it already and you're just looking for a
rubberstamp from me, that's ok too (Dale can just let me know :).

[Peng, 2016-03-07 23:49:10]

On 2016/03/07 23:00:01, raymes wrote:
> I'm not so familiar with this pepper code. +bbudge or penghuang
> 
> Or if Dale has had a look at it already and you're just looking for a
> rubberstamp from me, that's ok too (Dale can just let me know :).

content/renderer/pepper/pepper_media_stream_video_track_host.cc LGTM

For content/renderer/pepper/pepper_video_source_host.cc, it is better to ask
bbudge to take a look.

[emircan, 2016-03-07 23:54:22]

Patchset #5 (id:120001) has been deleted

[emircan, 2016-03-08 01:08:03]

Description was changed from

==========
Handle Alpha channel in Canvas capture

This CL changes the output frame format in canvas capture from
I420 to YV12A so that alpha channel is preserved. The change
discussion is here [0].

As a result, each MediaStreamSink would be responsible for
handling YV12A frames. For that purpose 
VideoUtil::WrapAsI420VideoFrame() is added to each appropriate
sink.

[0] https://github.com/w3c/mediacapture-fromelement/issues/30
[1]
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/ren...
BUG=524218
TEST= https://cdn.rawgit.com/uysalere/js-demos/master/canvascapture3.html
==========

to

==========
Handle Alpha channel in Canvas capture

This CL changes the output frame format in canvas capture from
I420 to YV12A so that alpha channel is preserved. The change
discussion is here [0].

As a result, each MediaStreamSink would be responsible for
handling YV12A frames. For that purpose 
VideoUtil::WrapAsI420VideoFrame() is added to each appropriate
sink.

[0] https://github.com/w3c/mediacapture-fromelement/issues/30
[1]
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/ren...
BUG=524218
TEST= https://cdn.rawgit.com/uysalere/js-demos/master/canvascapture3.html
TBR=bbudge@chromium.org
==========

[emircan, 2016-03-08 02:46:23]

Thanks. 

bbudge@ seems is marked as OOO until 3-18 so I will add him to TBR.

[emircan, 2016-03-08 02:51:22]

The CQ bit was checked by emircan@chromium.org

[emircan, 2016-03-08 02:51:23]

The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org,
dalecurtis@chromium.org, penghuang@chromium.org
Link to the patchset: https://codereview.chromium.org/1737253002/#ps140001
(title: "Rebase")

[commit-bot: I haz the power, 2016-03-08 02:51:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1737253002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1737253002/140001

[raymes, 2016-03-08 03:38:48]

The CQ bit was unchecked by raymes@chromium.org

[raymes, 2016-03-08 03:48:35]

On 2016/03/08 02:46:23, emircan wrote:
> Thanks. 
> 
> bbudge@ seems is marked as OOO until 3-18 so I will add him to TBR.

Sorry to un-CQ :( I think we should still ensure this gets the appropriate
approvals. As I said above I'm happy to approve as long as it's had a high level
review from someone who is best in the know (e.g. Dale or Peng?). But it was
unclear to me if that had actually happened or not based on esprehn's comments.

For TBR guidelines please see
http://www.chromium.org/developers/owners-files#TOC-When-to-use-To-Be-Reviewe...

[bbudge, 2016-03-08 13:17:33]

https://codereview.chromium.org/1737253002/diff/140001/content/renderer/peppe...
File content/renderer/pepper/pepper_media_stream_video_track_host.cc (right):

https://codereview.chromium.org/1737253002/diff/140001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_video_track_host.cc:383: 
nit: why add whitespace here?

https://codereview.chromium.org/1737253002/diff/140001/content/renderer/peppe...
File content/renderer/pepper/pepper_video_source_host.cc (right):

https://codereview.chromium.org/1737253002/diff/140001/content/renderer/peppe...
content/renderer/pepper/pepper_video_source_host.cc:65: video_frame->format() ==
media::PIXEL_FORMAT_YV12A)) {
Why is this checked here, but not in
content/renderer/pepper/pepper_media_stream_video_track_host.cc?

[emircan, 2016-03-08 19:01:46]

https://codereview.chromium.org/1737253002/diff/140001/content/renderer/peppe...
File content/renderer/pepper/pepper_media_stream_video_track_host.cc (right):

https://codereview.chromium.org/1737253002/diff/140001/content/renderer/peppe...
content/renderer/pepper/pepper_media_stream_video_track_host.cc:383: 
On 2016/03/08 13:17:33, bbudge (OOO until 3-18) wrote:
> nit: why add whitespace here?

Removed

https://codereview.chromium.org/1737253002/diff/140001/content/renderer/peppe...
File content/renderer/pepper/pepper_video_source_host.cc (right):

https://codereview.chromium.org/1737253002/diff/140001/content/renderer/peppe...
content/renderer/pepper/pepper_video_source_host.cc:65: video_frame->format() ==
media::PIXEL_FORMAT_YV12A)) {
On 2016/03/08 13:17:33, bbudge (OOO until 3-18) wrote:
> Why is this checked here, but not in
> content/renderer/pepper/pepper_media_stream_video_track_host.cc?

With this patch, each sink is expected to handle a new format. While doing that,
I wanted to also make it very clear which formats are expected as input so I
included these checks. 

In PepperMediaStreamVideoTrackHost::OnVideoFrame, ToPpapiFormat method checks
the format[0] and handles unexpected formats. I didn't want to change that
logic. Still, if you think I should remove the checks here or add it there, I
can do that. WDYT?

[0]
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p...

[bbudge, 2016-03-08 20:13:34]

This API is private and only used by Hangouts special effects (adds mustache,
hats, etc. to videos). You might want to make sure this patch doesn't break
that.

Otherwise, LGTM.

https://codereview.chromium.org/1737253002/diff/140001/content/renderer/peppe...
File content/renderer/pepper/pepper_video_source_host.cc (right):

https://codereview.chromium.org/1737253002/diff/140001/content/renderer/peppe...
content/renderer/pepper/pepper_video_source_host.cc:65: video_frame->format() ==
media::PIXEL_FORMAT_YV12A)) {
On 2016/03/08 19:01:46, emircan wrote:
> On 2016/03/08 13:17:33, bbudge (OOO until 3-18) wrote:
> > Why is this checked here, but not in
> > content/renderer/pepper/pepper_media_stream_video_track_host.cc?
> 
> With this patch, each sink is expected to handle a new format. While doing
that,
> I wanted to also make it very clear which formats are expected as input so I
> included these checks. 
> 
> In PepperMediaStreamVideoTrackHost::OnVideoFrame, ToPpapiFormat method checks
> the format[0] and handles unexpected formats. I didn't want to change that
> logic. Still, if you think I should remove the checks here or add it there, I
> can do that. WDYT?
> 
> [0]
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p...

OK, this is fine then.

[emircan, 2016-03-08 21:43:42]

Thanks. I tested hangouts with effects and verified as well.

[emircan, 2016-03-08 21:44:05]

The CQ bit was checked by emircan@chromium.org

[emircan, 2016-03-08 21:44:06]

The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org,
dalecurtis@chromium.org, penghuang@chromium.org
Link to the patchset: https://codereview.chromium.org/1737253002/#ps160001
(title: "bbudge@ nit.")

[commit-bot: I haz the power, 2016-03-08 21:44:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1737253002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1737253002/160001

[commit-bot: I haz the power, 2016-03-08 22:13:59]

Description was changed from

==========
Handle Alpha channel in Canvas capture

This CL changes the output frame format in canvas capture from
I420 to YV12A so that alpha channel is preserved. The change
discussion is here [0].

As a result, each MediaStreamSink would be responsible for
handling YV12A frames. For that purpose 
VideoUtil::WrapAsI420VideoFrame() is added to each appropriate
sink.

[0] https://github.com/w3c/mediacapture-fromelement/issues/30
[1]
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/ren...
BUG=524218
TEST= https://cdn.rawgit.com/uysalere/js-demos/master/canvascapture3.html
TBR=bbudge@chromium.org
==========

to

==========
Handle Alpha channel in Canvas capture

This CL changes the output frame format in canvas capture from
I420 to YV12A so that alpha channel is preserved. The change
discussion is here [0].

As a result, each MediaStreamSink would be responsible for
handling YV12A frames. For that purpose 
VideoUtil::WrapAsI420VideoFrame() is added to each appropriate
sink.

[0] https://github.com/w3c/mediacapture-fromelement/issues/30
[1]
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/ren...
BUG=524218
TEST= https://cdn.rawgit.com/uysalere/js-demos/master/canvascapture3.html
TBR=bbudge@chromium.org
==========

[commit-bot: I haz the power, 2016-03-08 22:14:01]

Committed patchset #6 (id:160001)

[commit-bot: I haz the power, 2016-03-08 22:15:17]

Description was changed from

==========
Handle Alpha channel in Canvas capture

This CL changes the output frame format in canvas capture from
I420 to YV12A so that alpha channel is preserved. The change
discussion is here [0].

As a result, each MediaStreamSink would be responsible for
handling YV12A frames. For that purpose 
VideoUtil::WrapAsI420VideoFrame() is added to each appropriate
sink.

[0] https://github.com/w3c/mediacapture-fromelement/issues/30
[1]
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/ren...
BUG=524218
TEST= https://cdn.rawgit.com/uysalere/js-demos/master/canvascapture3.html
TBR=bbudge@chromium.org
==========

to

==========
Handle Alpha channel in Canvas capture

This CL changes the output frame format in canvas capture from
I420 to YV12A so that alpha channel is preserved. The change
discussion is here [0].

As a result, each MediaStreamSink would be responsible for
handling YV12A frames. For that purpose
VideoUtil::WrapAsI420VideoFrame() is added to each appropriate
sink.

[0] https://github.com/w3c/mediacapture-fromelement/issues/30
[1]
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/ren...
BUG=524218
TEST= https://cdn.rawgit.com/uysalere/js-demos/master/canvascapture3.html
TBR=bbudge@chromium.org

Committed: https://crrev.com/ec1e7ff3b75787d3290ba5a899db8ef4ca79d302
Cr-Commit-Position: refs/heads/master@{#379932}
==========

[commit-bot: I haz the power, 2016-03-08 22:15:19]

Patchset 6 (id:??) landed as
https://crrev.com/ec1e7ff3b75787d3290ba5a899db8ef4ca79d302
Cr-Commit-Position: refs/heads/master@{#379932}