Replace media::VideoCapture::VideoFrameBuffer with media::VideoFrame.

Replace media::VideoCapture::VideoFrameBuffer with media::VideoFrame.

This CL removes a redundant class in the form of
media::VideoCapture::VideoFrameBuffer, and replaces it with
media::VideoFrame.  Also: as media::VideoFrame has an embedded
destruction callback, use this to handle buffer returns on
the video capture stack, and remove the explicit
VideoCapture::FeedBuffer entry point.

BUG=None
TEST=local build, content_unittests, run on desktop Linux with
     screen capture and webcam

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

[sheu, 2013-09-11 06:51:29]

miu@: initial eyes-on the patch.  PTAL

[ncarter (slow), 2013-09-11 18:04:56]

LGTM. It might be worth mentioning in the CL description what happened to
FeedBuffer.

https://chromiumcodereview.appspot.com/23587018/diff/3001/content/renderer/me...
File content/renderer/media/video_capture_impl.cc (right):

https://chromiumcodereview.appspot.com/23587018/diff/3001/content/renderer/me...
content/renderer/media/video_capture_impl.cc:291:
media::VideoFrame::WrapExternalSharedMemory(
Exciting. This is gonna work great with
https://codereview.chromium.org/23551011/ .

https://chromiumcodereview.appspot.com/23587018/diff/3001/content/renderer/pe...
File content/renderer/pepper/pepper_video_capture_host.cc (right):

https://chromiumcodereview.appspot.com/23587018/diff/3001/content/renderer/pe...
content/renderer/pepper/pepper_video_capture_host.cc:147: ++j) {
This loop is correct, but it kind of opaquely relies on the fact that the enum
ordering in media::VideoFrame::Format is Y/U/V (if those values were declared in
a different order, you could wind up with YV12 instead of I420 here). Would it
be overly paranoid to add a compile time assert here?

[sheu, 2013-09-11 19:28:10]

miu@: PTAL.  If it's all right with you then I'll start hunting for OWNERs.

https://chromiumcodereview.appspot.com/23587018/diff/3001/content/renderer/pe...
File content/renderer/pepper/pepper_video_capture_host.cc (right):

https://chromiumcodereview.appspot.com/23587018/diff/3001/content/renderer/pe...
content/renderer/pepper/pepper_video_capture_host.cc:147: ++j) {
On 2013/09/11 18:04:56, ncarter wrote:
> This loop is correct, but it kind of opaquely relies on the fact that the enum
> ordering in media::VideoFrame::Format is Y/U/V (if those values were declared
in
> a different order, you could wind up with YV12 instead of I420 here). Would it
> be overly paranoid to add a compile time assert here?

I DCHECKED for I420 above, so this is safe for now.  I'd like to clean up the
plane copies in/out of media::VideoFrame, but that's another changelist.

[sheu, 2013-09-11 19:29:19]

ihf@: PTAL at the content/renderer/pepper/* bits.

[ncarter (slow), 2013-09-11 19:56:07]

https://chromiumcodereview.appspot.com/23587018/diff/3001/content/renderer/pe...
File content/renderer/pepper/pepper_video_capture_host.cc (right):

https://chromiumcodereview.appspot.com/23587018/diff/3001/content/renderer/pe...
content/renderer/pepper/pepper_video_capture_host.cc:147: ++j) {
On 2013/09/11 19:28:10, sheu wrote:
> On 2013/09/11 18:04:56, ncarter wrote:
> > This loop is correct, but it kind of opaquely relies on the fact that the
enum
> > ordering in media::VideoFrame::Format is Y/U/V (if those values were
declared
> in
> > a different order, you could wind up with YV12 instead of I420 here). Would
it
> > be overly paranoid to add a compile time assert here?
> 
> I DCHECKED for I420 above, so this is safe for now.  I'd like to clean up the
> plane copies in/out of media::VideoFrame, but that's another changelist.

The I420 DCHECK is on the source. The thing that's fragile is that the dest also
wants I420 (e.g. if the source were YV12, this code would produce an I420
destination).

[ilja, 2013-09-12 01:31:43]

I spoke with John and it looks fine to me.

https://chromiumcodereview.appspot.com/23587018/diff/3001/content/renderer/pe...
File content/renderer/pepper/pepper_video_capture_host.cc (right):

https://chromiumcodereview.appspot.com/23587018/diff/3001/content/renderer/pe...
content/renderer/pepper/pepper_video_capture_host.cc:147: ++j) {
On 2013/09/11 19:56:08, ncarter wrote:
> On 2013/09/11 19:28:10, sheu wrote:
> > On 2013/09/11 18:04:56, ncarter wrote:
> > > This loop is correct, but it kind of opaquely relies on the fact that the
> enum
> > > ordering in media::VideoFrame::Format is Y/U/V (if those values were
> declared
> > in
> > > a different order, you could wind up with YV12 instead of I420 here).
Would
> it
> > > be overly paranoid to add a compile time assert here?
> > 
> > I DCHECKED for I420 above, so this is safe for now.  I'd like to clean up
the
> > plane copies in/out of media::VideoFrame, but that's another changelist.
> 
> The I420 DCHECK is on the source. The thing that's fragile is that the dest
also
> wants I420 (e.g. if the source were YV12, this code would produce an I420
> destination).

It is not specified anywhere, but right now Flash expects I420 and nothing else.

[miu, 2013-09-12 03:01:57]

https://chromiumcodereview.appspot.com/23587018/diff/3001/content/renderer/me...
File content/renderer/media/rtc_video_capturer.cc (right):

https://chromiumcodereview.appspot.com/23587018/diff/3001/content/renderer/me...
content/renderer/media/rtc_video_capturer.cc:43: start_time_delta_ =
base::Time::Now() - base::Time::UnixEpoch();
For consideration: Instead of doing this here, you might instead initialize
start_time_delta_ on the very first call to OnFrameCaptured().  This would
completely eliminate the read of the system clock.

Meaning, replace this line of code with:

  has_captured_first_frame_ = false;

and then in OnFrameCaptured(), just before assigning to
cricket_frame.elapsed_time:

  if (!has_captured_first_frame_) {
    start_time_delta_ = frame->GetTimestamp();
    has_captured_first_frame_ = true;
  }

https://chromiumcodereview.appspot.com/23587018/diff/3001/content/renderer/pe...
File content/renderer/pepper/pepper_video_capture_host.cc (right):

https://chromiumcodereview.appspot.com/23587018/diff/3001/content/renderer/pe...
content/renderer/pepper/pepper_video_capture_host.cc:139: if
(static_cast<size_t>(buffers_[i].buffer->size() <
Need to place the closing parenthesis of the static_cast expression before the
"<" operator.  However, if the compiler didn't complain about this code as it
is, maybe you don't need the static_cast at all?

[sheu, 2013-09-12 18:47:42]

Updated.  PTAL

https://chromiumcodereview.appspot.com/23587018/diff/3001/content/renderer/pe...
File content/renderer/pepper/pepper_video_capture_host.cc (right):

https://chromiumcodereview.appspot.com/23587018/diff/3001/content/renderer/pe...
content/renderer/pepper/pepper_video_capture_host.cc:139: if
(static_cast<size_t>(buffers_[i].buffer->size() <
On 2013/09/12 03:01:58, Yuri wrote:
> Need to place the closing parenthesis of the static_cast expression before the
> "<" operator.  However, if the compiler didn't complain about this code as it
> is, maybe you don't need the static_cast at all?

Done.

https://chromiumcodereview.appspot.com/23587018/diff/3001/content/renderer/pe...
content/renderer/pepper/pepper_video_capture_host.cc:147: ++j) {
On 2013/09/12 01:31:43, ilja wrote:
> On 2013/09/11 19:56:08, ncarter wrote:
> > On 2013/09/11 19:28:10, sheu wrote:
> > > On 2013/09/11 18:04:56, ncarter wrote:
> > > > This loop is correct, but it kind of opaquely relies on the fact that
the
> > enum
> > > > ordering in media::VideoFrame::Format is Y/U/V (if those values were
> > declared
> > > in
> > > > a different order, you could wind up with YV12 instead of I420 here).
> Would
> > it
> > > > be overly paranoid to add a compile time assert here?
> > > 
> > > I DCHECKED for I420 above, so this is safe for now.  I'd like to clean up
> the
> > > plane copies in/out of media::VideoFrame, but that's another changelist.
> > 
> > The I420 DCHECK is on the source. The thing that's fragile is that the dest
> also
> > wants I420 (e.g. if the source were YV12, this code would produce an I420
> > destination).
> 
> It is not specified anywhere, but right now Flash expects I420 and nothing
else.

Done.

[sheu, 2013-09-12 18:49:52]

Adding OWNERS.

xhwang@: PTAL at content/renderer/media/*
yzshen@: PTAL at content/renderer/pepper/*
fischman@: PTAL at media/capture/*

[miu, 2013-09-12 19:29:20]

lgtm, but with one concern:

https://chromiumcodereview.appspot.com/23587018/diff/15001/content/renderer/m...
File content/renderer/media/rtc_video_capturer.cc (right):

https://chromiumcodereview.appspot.com/23587018/diff/15001/content/renderer/m...
content/renderer/media/rtc_video_capturer.cc:44: delegate_->StartCapture(
Can the RtcVideoCapturer be stopped and then started again?  If the answer is
no, then never mind.  However, the previous behavior was that start_time_delta_
was being re-assigned for every call to Start().

[sheu, 2013-09-12 21:37:10]

Addressed miu@'s comment.  OWNERS: start your engines.

https://chromiumcodereview.appspot.com/23587018/diff/15001/content/renderer/m...
File content/renderer/media/rtc_video_capturer.cc (right):

https://chromiumcodereview.appspot.com/23587018/diff/15001/content/renderer/m...
content/renderer/media/rtc_video_capturer.cc:44: delegate_->StartCapture(
On 2013/09/12 19:29:20, Yuri wrote:
> Can the RtcVideoCapturer be stopped and then started again?  If the answer is
> no, then never mind.  However, the previous behavior was that
start_time_delta_
> was being re-assigned for every call to Start().

Done.

[Ami GONE FROM CHROMIUM, 2013-09-12 22:40:40]

https://chromiumcodereview.appspot.com/23587018/diff/23001/content/renderer/m...
File content/renderer/media/rtc_video_capture_delegate.cc (left):

https://chromiumcodereview.appspot.com/23587018/diff/23001/content/renderer/m...
content/renderer/media/rtc_video_capture_delegate.cc:115:
capture->FeedBuffer(buf);
lol I wrote this:
  Nick: I think this is right but I want you to make sure.
and then looked over the reviewlog from before I was added to the CL and see
that Nick suggested you add something to the CL description 'splaining this
change.  Please do do that.
(and in case you're one of those crazy cros folks who is used to gerrit,
remember that editing your git commit messages has no effect on rietveld; you
must edit the CL description on rietveld itself)

https://chromiumcodereview.appspot.com/23587018/diff/23001/content/renderer/m...
File content/renderer/media/rtc_video_capturer.cc (right):

https://chromiumcodereview.appspot.com/23587018/diff/23001/content/renderer/m...
content/renderer/media/rtc_video_capturer.cc:107: cricket::CapturedFrame
cricket_frame;
your attempts at disambiguation are no match for the twisty maze of frames in
this codebase!!!  
Behold:
You have named your cricket::CapturedFrame by the cute moniker of cricket_frame.
BUT there is also a cricket::VideoFrame type.
"cricket_frame" fails to distinguish between the cricket::{Captured,Video}Frame
types.

I (mildly) suggest captured_frame.

https://chromiumcodereview.appspot.com/23587018/diff/23001/content/renderer/m...
content/renderer/media/rtc_video_capturer.cc:117: // TODO(sheu): we assume
contiguous layout of image planes.
then the pepper code can be simplified even more, no?

https://chromiumcodereview.appspot.com/23587018/diff/23001/content/renderer/m...
File content/renderer/media/rtc_video_capturer.h (right):

https://chromiumcodereview.appspot.com/23587018/diff/23001/content/renderer/m...
content/renderer/media/rtc_video_capturer.h:52: bool
first_frame_timestamp_valid_;
FWIW, media/base/buffers.h:kNoTimestamp() is a thing that exists, which you
could use as a sentinel value instead of tracking this extra variable.  I don't
really care which you do.

https://chromiumcodereview.appspot.com/23587018/diff/23001/content/renderer/m...
File content/renderer/media/video_capture_impl.cc (right):

https://chromiumcodereview.appspot.com/23587018/diff/23001/content/renderer/m...
content/renderer/media/video_capture_impl.cc:268: iter,
nit: It's a bit strange to be using iter given you know it's
client_buffers_.end().
You really don't think it's more readable as
client_buffers_[buffer_id] = make_scoped_refptr(...); 
?

https://chromiumcodereview.appspot.com/23587018/diff/23001/content/renderer/m...
content/renderer/media/video_capture_impl.cc:302: base::Bind(
You might enjoy media::BindToLoop().

https://chromiumcodereview.appspot.com/23587018/diff/23001/content/renderer/p...
File content/renderer/pepper/pepper_video_capture_host.cc (right):

https://chromiumcodereview.appspot.com/23587018/diff/23001/content/renderer/p...
content/renderer/pepper/pepper_video_capture_host.cc:145:
COMPILE_ASSERT(media::VideoFrame::kYPlane == 0, y_plane_should_be_0);
What's the point of these tests, beyond the I420 DCHECK above?

https://chromiumcodereview.appspot.com/23587018/diff/23001/content/renderer/p...
content/renderer/pepper/pepper_video_capture_host.cc:147:
COMPILE_ASSERT(media::VideoFrame::kVPlane == 2, v_plane_should_be_2);
lol I've never wondered this before, but what does it really _mean_ if format is
YV12, but U precedes V as it always does in plane-index constants??

https://chromiumcodereview.appspot.com/23587018/diff/23001/content/renderer/p...
content/renderer/pepper/pepper_video_capture_host.cc:148: for (size_t j = 0; j <
media::VideoFrame::NumPlanes(frame->format());
I idly wonder if there value in a 
void VideoFrame::CopyToI420(void* dst);
helper.

https://chromiumcodereview.appspot.com/23587018/diff/23001/content/renderer/p...
content/renderer/pepper/pepper_video_capture_host.cc:157: }
Is the sole difference between l.150-157 and:

size_t size = frame->stride(j)*frame->rows(j);
memcpy(dst, frame->data(j), size);
dst += size;

that you avoid copying the padding bytes?  Is that worthwhile? (IDK)

[sheu, 2013-09-13 00:15:18]

https://chromiumcodereview.appspot.com/23587018/diff/23001/content/renderer/m...
File content/renderer/media/rtc_video_capture_delegate.cc (left):

https://chromiumcodereview.appspot.com/23587018/diff/23001/content/renderer/m...
content/renderer/media/rtc_video_capture_delegate.cc:115:
capture->FeedBuffer(buf);
On 2013/09/12 22:40:41, Ami Fischman wrote:
> lol I wrote this:
>   Nick: I think this is right but I want you to make sure.
> and then looked over the reviewlog from before I was added to the CL and see
> that Nick suggested you add something to the CL description 'splaining this
> change.  Please do do that.
> (and in case you're one of those crazy cros folks who is used to gerrit,
> remember that editing your git commit messages has no effect on rietveld; you
> must edit the CL description on rietveld itself)

Done.

https://chromiumcodereview.appspot.com/23587018/diff/23001/content/renderer/m...
File content/renderer/media/rtc_video_capturer.cc (right):

https://chromiumcodereview.appspot.com/23587018/diff/23001/content/renderer/m...
content/renderer/media/rtc_video_capturer.cc:117: // TODO(sheu): we assume
contiguous layout of image planes.
On 2013/09/12 22:40:41, Ami Fischman wrote:
> then the pepper code can be simplified even more, no?

yes, but I'd like not to make that assumption explicitly, as long as I can avoid
it.

It's a little hacky here since I'm making the assumption.

https://chromiumcodereview.appspot.com/23587018/diff/23001/content/renderer/m...
File content/renderer/media/rtc_video_capturer.h (right):

https://chromiumcodereview.appspot.com/23587018/diff/23001/content/renderer/m...
content/renderer/media/rtc_video_capturer.h:52: bool
first_frame_timestamp_valid_;
On 2013/09/12 22:40:41, Ami Fischman wrote:
> FWIW, media/base/buffers.h:kNoTimestamp() is a thing that exists, which you
> could use as a sentinel value instead of tracking this extra variable.  I
don't
> really care which you do.

That actually makes more sense.  Done.

https://chromiumcodereview.appspot.com/23587018/diff/23001/content/renderer/m...
File content/renderer/media/video_capture_impl.cc (right):

https://chromiumcodereview.appspot.com/23587018/diff/23001/content/renderer/m...
content/renderer/media/video_capture_impl.cc:268: iter,
On 2013/09/12 22:40:41, Ami Fischman wrote:
> nit: It's a bit strange to be using iter given you know it's
> client_buffers_.end().
> You really don't think it's more readable as
> client_buffers_[buffer_id] = make_scoped_refptr(...); 
> ?

I got confused about insert-with-hint.  :-(

https://chromiumcodereview.appspot.com/23587018/diff/23001/content/renderer/m...
content/renderer/media/video_capture_impl.cc:302: base::Bind(
On 2013/09/12 22:40:41, Ami Fischman wrote:
> You might enjoy media::BindToLoop().

Is there any reason why this isn't in base:: ?

we're binding for a media::VideoFrame here, so I'm feel ok.  In another context
I might not.

https://chromiumcodereview.appspot.com/23587018/diff/23001/content/renderer/p...
File content/renderer/pepper/pepper_video_capture_host.cc (right):

https://chromiumcodereview.appspot.com/23587018/diff/23001/content/renderer/p...
content/renderer/pepper/pepper_video_capture_host.cc:145:
COMPILE_ASSERT(media::VideoFrame::kYPlane == 0, y_plane_should_be_0);
On 2013/09/12 22:40:41, Ami Fischman wrote:
> What's the point of these tests, beyond the I420 DCHECK above?

'Cause ncarter@ wanted them?

We go through the plane indices below in a dumb loop, so I'm just checking that
they're what we expect.

https://chromiumcodereview.appspot.com/23587018/diff/23001/content/renderer/p...
content/renderer/pepper/pepper_video_capture_host.cc:147:
COMPILE_ASSERT(media::VideoFrame::kVPlane == 2, v_plane_should_be_2);
On 2013/09/12 22:40:41, Ami Fischman wrote:
> lol I've never wondered this before, but what does it really _mean_ if format
is
> YV12, but U precedes V as it always does in plane-index constants??

It means that the data pointers we get out of media::VideoFrame::data() will be
out-of-order w.r.t. the planes for YV12, since we expect YUV ordering.

I've been doing a lot of contiguous plane <-> media::VideoFrame conversions
lately, and it makes me feel dirty.

https://chromiumcodereview.appspot.com/23587018/diff/23001/content/renderer/p...
content/renderer/pepper/pepper_video_capture_host.cc:148: for (size_t j = 0; j <
media::VideoFrame::NumPlanes(frame->format());
On 2013/09/12 22:40:41, Ami Fischman wrote:
> I idly wonder if there value in a 
> void VideoFrame::CopyToI420(void* dst);
> helper.

I looked already, and so far this is the only place we do it (!)

https://chromiumcodereview.appspot.com/23587018/diff/23001/content/renderer/p...
content/renderer/pepper/pepper_video_capture_host.cc:157: }
On 2013/09/12 22:40:41, Ami Fischman wrote:
> Is the sole difference between l.150-157 and:
> 
> size_t size = frame->stride(j)*frame->rows(j);
> memcpy(dst, frame->data(j), size);
> dst += size;
> 
> that you avoid copying the padding bytes?  Is that worthwhile? (IDK)

Paranoia in case strides don't match up.

[sheu, 2013-09-13 00:15:34]

Updated.  PTAL

[Ami GONE FROM CHROMIUM, 2013-09-13 00:26:34]

LGTM

https://chromiumcodereview.appspot.com/23587018/diff/23001/content/renderer/p...
File content/renderer/pepper/pepper_video_capture_host.cc (right):

https://chromiumcodereview.appspot.com/23587018/diff/23001/content/renderer/p...
content/renderer/pepper/pepper_video_capture_host.cc:148: for (size_t j = 0; j <
media::VideoFrame::NumPlanes(frame->format());
On 2013/09/13 00:15:19, sheu wrote:
> On 2013/09/12 22:40:41, Ami Fischman wrote:
> > I idly wonder if there value in a 
> > void VideoFrame::CopyToI420(void* dst);
> > helper.
> 
> I looked already, and so far this is the only place we do it (!)

!!

https://chromiumcodereview.appspot.com/23587018/diff/22001/content/renderer/m...
File content/renderer/media/video_capture_impl.cc (right):

https://chromiumcodereview.appspot.com/23587018/diff/22001/content/renderer/m...
content/renderer/media/video_capture_impl.cc:259:
DCHECK(client_buffers_.find(buffer_id) == client_buffers_.end());
fwiw, 
mymap.find(key) == mymap.end()
is equivalent to but longer than
mymap.count(key) == 0
or even
!mymap.count(key)
just sayin'.

For my money, if what you're doing is "make sure my insert is neither a no-op
nor a silent overwrite" then the best idiom is
bool inserted = mymap.insert(make_pair(key, value));
DCHECK(inserted);
because it involves only a single walk of the map, and says what it does instead
of breaking it up into multiple statements whose interrelation may be less than
clear.

But obvs. this is all personal taste.

https://chromiumcodereview.appspot.com/23587018/diff/22001/content/renderer/m...
content/renderer/media/video_capture_impl.cc:299: media::BindToLoop(
On 2013/09/13 00:15:19, sheu wrote:
> On 2013/09/12 22:40:41, Ami Fischman wrote:
> > You might enjoy media::BindToLoop().
> 
> Is there any reason why this isn't in base:: ?

Only a lack of faith by base/bind's OWNERS that this would be a generally useful
facility when I first introduced it.

https://chromiumcodereview.appspot.com/23587018/diff/22001/content/renderer/p...
File content/renderer/pepper/pepper_video_capture_host.cc (right):

https://chromiumcodereview.appspot.com/23587018/diff/22001/content/renderer/p...
content/renderer/pepper/pepper_video_capture_host.cc:158: }
On 2013/09/13 00:15:19, sheu wrote:
> On 2013/09/12 22:40:41, Ami Fischman wrote:
> > Is the sole difference between l.150-157 and:
> > 
> > size_t size = frame->stride(j)*frame->rows(j);
> > memcpy(dst, frame->data(j), size);
> > dst += size;
> > 
> > that you avoid copying the padding bytes?  Is that worthwhile? (IDK)
> 
> Paranoia in case strides don't match up.

I missed that this removes stride padding(!).

[ncarter (slow), 2013-09-13 21:01:11]

https://chromiumcodereview.appspot.com/23587018/diff/23001/content/renderer/p...
File content/renderer/pepper/pepper_video_capture_host.cc (right):

https://chromiumcodereview.appspot.com/23587018/diff/23001/content/renderer/p...
content/renderer/pepper/pepper_video_capture_host.cc:148: for (size_t j = 0; j <
media::VideoFrame::NumPlanes(frame->format());
On 2013/09/13 00:26:34, Ami Fischman wrote:
> On 2013/09/13 00:15:19, sheu wrote:
> > On 2013/09/12 22:40:41, Ami Fischman wrote:
> > > I idly wonder if there value in a 
> > > void VideoFrame::CopyToI420(void* dst);
> > > helper.
> > 
> > I looked already, and so far this is the only place we do it (!)
> 
> !!

Another idea would be to wrap the target pointer in a VideoFrame temporarily,
and use the existing Copy method.

VideoFrame -- it's SkBitmap, essentially.

[yzshen1, 2013-09-16 16:34:16]

pepper OWNER LGTM

[sheu, 2013-09-16 19:09:11]

https://chromiumcodereview.appspot.com/23587018/diff/23001/content/renderer/p...
File content/renderer/pepper/pepper_video_capture_host.cc (right):

https://chromiumcodereview.appspot.com/23587018/diff/23001/content/renderer/p...
content/renderer/pepper/pepper_video_capture_host.cc:148: for (size_t j = 0; j <
media::VideoFrame::NumPlanes(frame->format());
On 2013/09/13 21:01:11, ncarter wrote:
> On 2013/09/13 00:26:34, Ami Fischman wrote:
> > On 2013/09/13 00:15:19, sheu wrote:
> > > On 2013/09/12 22:40:41, Ami Fischman wrote:
> > > > I idly wonder if there value in a 
> > > > void VideoFrame::CopyToI420(void* dst);
> > > > helper.
> > > 
> > > I looked already, and so far this is the only place we do it (!)
> > 
> > !!
> 
> Another idea would be to wrap the target pointer in a VideoFrame temporarily,
> and use the existing Copy method.
> 
> VideoFrame -- it's SkBitmap, essentially.

A little heavyweight there, I think.

https://chromiumcodereview.appspot.com/23587018/diff/22001/content/renderer/m...
File content/renderer/media/video_capture_impl.cc (right):

https://chromiumcodereview.appspot.com/23587018/diff/22001/content/renderer/m...
content/renderer/media/video_capture_impl.cc:259:
DCHECK(client_buffers_.find(buffer_id) == client_buffers_.end());
On 2013/09/13 00:26:35, Ami Fischman wrote:
> fwiw, 
> mymap.find(key) == mymap.end()
> is equivalent to but longer than
> mymap.count(key) == 0
> or even
> !mymap.count(key)
> just sayin'.
> 
> For my money, if what you're doing is "make sure my insert is neither a no-op
> nor a silent overwrite" then the best idiom is
> bool inserted = mymap.insert(make_pair(key, value));
> DCHECK(inserted);
> because it involves only a single walk of the map, and says what it does
instead
> of breaking it up into multiple statements whose interrelation may be less
than
> clear.
> 
> But obvs. this is all personal taste.

Sure, this way works.

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

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

[commit-bot: I haz the power, 2013-09-16 19:26:06]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2013-09-16 20:20:33]

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

[commit-bot: I haz the power, 2013-09-16 21:58:44]

Change committed as 223434