Update I420Buffer to new VideoFrameBuffer interface

Update I420Buffer to new VideoFrameBuffer interface

This is a follow-up cleanup for CL https://codereview.webrtc.org/2847383002/.

BUG=webrtc:7632
TBR=stefan

Review-Url: https://codereview.webrtc.org/2906053002
Cr-Commit-Position: refs/heads/master@{#18388}
Committed: https://chromium.googlesource.com/external/webrtc/+/3f075498a3a3d7426deded720e21a6f77d1cabef

[magjed_webrtc, 2017-05-26 12:28:09]

Description was changed from

==========
Update I420Buffer to new VideoFrameBuffer interface

BUG=webrtc:7632
==========

to

==========
Update I420Buffer to new VideoFrameBuffer interface

This is a follow-up cleanup for CL https://codereview.webrtc.org/2847383002/.

BUG=webrtc:7632
==========

[magjed_webrtc, 2017-05-26 12:28:24]

The CQ bit was checked by magjed@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-26 12:28:26]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-05-26 12:32:49]

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

[commit-bot: I haz the power, 2017-05-26 12:32:49]

Dry run: Try jobs failed on following builders:
  ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...)
  linux32_arm_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux32_arm_dbg/builds/8)
  mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)

[magjed_webrtc, 2017-05-26 12:40:25]

The CQ bit was checked by magjed@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-26 12:40:30]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-05-26 12:46:42]

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

[commit-bot: I haz the power, 2017-05-26 12:46:42]

Dry run: Try jobs failed on following builders:
  mac_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/25968)

[magjed_webrtc, 2017-05-26 13:27:19]

The CQ bit was checked by magjed@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-26 13:27:26]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[magjed_webrtc, 2017-05-26 13:34:59]

Patchset #1 (id:1) has been deleted

[magjed_webrtc, 2017-05-26 13:37:43]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-05-26 13:58:06]

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

[commit-bot: I haz the power, 2017-05-26 13:58:07]

Dry run: This issue passed the CQ dry run.

[magjed_webrtc, 2017-05-26 14:04:15]

magjed@webrtc.org changed reviewers:
+ nisse@webrtc.org

[magjed_webrtc, 2017-05-26 14:05:54]

nisse - please take a look.

There are more places to fix with the new VideoFrameBuffer interface, but I'll
address that in separeate CL:s.

[nisse-webrtc, 2017-05-29 08:22:23]

Looks pretty good. I have a couple of questions.

https://codereview.webrtc.org/2906053002/diff/40001/webrtc/api/video/i420_buf...
File webrtc/api/video/i420_buffer.cc (right):

https://codereview.webrtc.org/2906053002/diff/40001/webrtc/api/video/i420_buf...
webrtc/api/video/i420_buffer.cc:108: RTC_CHECK(src.type() == Type::kI420);
Hmm. To get better compile time type-checking, could we add some abstract
classes for I420 and I444 buffers, which would inherit PlanarYuvBuffer and
define the type() method only (and possibly define it using |final|).

Would be used for the input to functions like this which require I420, and the
GetI40, ToI420 and GetI444 methods should return a pointer of the right type,
and likewise should the I420BufferPool, etc.

Not sure if it can work out nicely, but what do you think?

https://codereview.webrtc.org/2906053002/diff/40001/webrtc/api/video/video_fr...
File webrtc/api/video/video_frame.h (right):

https://codereview.webrtc.org/2906053002/diff/40001/webrtc/api/video/video_fr...
webrtc/api/video/video_frame.h:101: return video_frame_buffer()->type() ==
VideoFrameBuffer::Type::kNative;
Do you agree this method should be deleted?

https://codereview.webrtc.org/2906053002/diff/40001/webrtc/media/base/adapted...
File webrtc/media/base/adaptedvideotracksource.cc (right):

https://codereview.webrtc.org/2906053002/diff/40001/webrtc/media/base/adapted...
webrtc/media/base/adaptedvideotracksource.cc:52:
webrtc::I420Buffer::Rotate(*buffer->GetI420(), frame.rotation()),
What's the right way to add support for rotating non-I420 frames here? It would
be nice to not have a |switch(buffer->type())|.

https://codereview.webrtc.org/2906053002/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/test/videoprocessor.cc (right):

https://codereview.webrtc.org/2906053002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/test/videoprocessor.cc:468:
scaled_buffer->ScaleFrom(*image.video_frame_buffer()->ToI420());
Nice improvement.

[magjed_webrtc, 2017-05-29 12:12:06]

https://codereview.webrtc.org/2906053002/diff/40001/webrtc/api/video/i420_buf...
File webrtc/api/video/i420_buffer.cc (right):

https://codereview.webrtc.org/2906053002/diff/40001/webrtc/api/video/i420_buf...
webrtc/api/video/i420_buffer.cc:108: RTC_CHECK(src.type() == Type::kI420);
On 2017/05/29 08:22:22, nisse-webrtc wrote:
> Hmm. To get better compile time type-checking, could we add some abstract
> classes for I420 and I444 buffers, which would inherit PlanarYuvBuffer and
> define the type() method only (and possibly define it using |final|).
> 
> Would be used for the input to functions like this which require I420, and the
> GetI40, ToI420 and GetI444 methods should return a pointer of the right type,
> and likewise should the I420BufferPool, etc.
> 
> Not sure if it can work out nicely, but what do you think?

Yeah, this is something I thought about when I did the first CL for exactly the
reasons you mention. I think it might be inflexible though to have a class for
'type() == Type::kI420', something that is currently dynamic. For example, when
implementing WrappedPlanarYuvBuffer, we can now send the type as an argument and
only need one class. If we make separate types for I420 and I444 we would need
two classes WrappedI420 and WrappedI444 in order to return different types, and
possible need to duplicate some logic for that.

I'm still on the fence, but if we want to change it, we should do it asap. What
do you think?

https://codereview.webrtc.org/2906053002/diff/40001/webrtc/api/video/video_fr...
File webrtc/api/video/video_frame.h (right):

https://codereview.webrtc.org/2906053002/diff/40001/webrtc/api/video/video_fr...
webrtc/api/video/video_frame.h:101: return video_frame_buffer()->type() ==
VideoFrameBuffer::Type::kNative;
On 2017/05/29 08:22:22, nisse-webrtc wrote:
> Do you agree this method should be deleted?

Yeah, but this function is not a big issue in my opinion.

https://codereview.webrtc.org/2906053002/diff/40001/webrtc/media/base/adapted...
File webrtc/media/base/adaptedvideotracksource.cc (right):

https://codereview.webrtc.org/2906053002/diff/40001/webrtc/media/base/adapted...
webrtc/media/base/adaptedvideotracksource.cc:52:
webrtc::I420Buffer::Rotate(*buffer->GetI420(), frame.rotation()),
On 2017/05/29 08:22:22, nisse-webrtc wrote:
> What's the right way to add support for rotating non-I420 frames here? It
would
> be nice to not have a |switch(buffer->type())|.

Hopefully we don't need to handle rotating non-I420 frames. Rotating buffers is
something we want to avoid, and it's fine for me to always use ToI420() when
apply_rotation() is true. In general, I prefer to switch on the type instead of
adding more virtual functions to VideoFrameBuffer.

[nisse-webrtc, 2017-05-29 12:37:18]

https://codereview.webrtc.org/2906053002/diff/40001/webrtc/api/video/i420_buf...
File webrtc/api/video/i420_buffer.cc (right):

https://codereview.webrtc.org/2906053002/diff/40001/webrtc/api/video/i420_buf...
webrtc/api/video/i420_buffer.cc:108: RTC_CHECK(src.type() == Type::kI420);
On 2017/05/29 12:12:06, magjed_webrtc wrote:
> On 2017/05/29 08:22:22, nisse-webrtc wrote:
> > Hmm. To get better compile time type-checking, could we add some abstract
> > classes for I420 and I444 buffers, which would inherit PlanarYuvBuffer and
> > define the type() method only (and possibly define it using |final|).

> Yeah, this is something I thought about when I did the first CL for exactly
the
> reasons you mention. I think it might be inflexible though to have a class for
> 'type() == Type::kI420', something that is currently dynamic.

Maybe we have to prepare some experimental cl to find out if it's feasible?

> For example, when
> implementing WrappedPlanarYuvBuffer, we can now send the type as an argument
and
> only need one class. If we make separate types for I420 and I444 we would need
> two classes WrappedI420 and WrappedI444 in order to return different types,

I guess one could avoid most code duplication by making the base type be a
template argument, but it might still be a bit awkward.

> I'm still on the fence, but if we want to change it, we should do it asap.
What
> do you think?

I think proper type checking for functions which require input arguments to be
I420 is a pretty strong reasong for doing this change. And it's hard to judge
the drawbacks without writing the code.

And we also have the hard naming problem... I420Buffer is taken, for the
implementation class. Should it be I420BufferInterface? That's not very pretty.

https://codereview.webrtc.org/2906053002/diff/40001/webrtc/media/base/adapted...
File webrtc/media/base/adaptedvideotracksource.cc (right):

https://codereview.webrtc.org/2906053002/diff/40001/webrtc/media/base/adapted...
webrtc/media/base/adaptedvideotracksource.cc:52:
webrtc::I420Buffer::Rotate(*buffer->GetI420(), frame.rotation()),
On 2017/05/29 12:12:06, magjed_webrtc wrote:
> On 2017/05/29 08:22:22, nisse-webrtc wrote:
> > What's the right way to add support for rotating non-I420 frames here? It
> would
> > be nice to not have a |switch(buffer->type())|.
> 
> Hopefully we don't need to handle rotating non-I420 frames. Rotating buffers
is
> something we want to avoid, and it's fine for me to always use ToI420() when
> apply_rotation() is true. In general, I prefer to switch on the type instead
of
> adding more virtual functions to VideoFrameBuffer.

Hmm, in the case of texture frames rotation mismatch here is always temporary
and we prefer dropping a frame or two over suddenly inserting a non-texture
frame into the pipeline. I guess we need the same to hold for I444 frames, or
else we'll pass I444 or I420 downstream depending on the current orientation.

Not sure what code depends on apply_rotation being handled here.

[magjed_webrtc, 2017-05-29 13:57:02]

https://codereview.webrtc.org/2906053002/diff/40001/webrtc/api/video/i420_buf...
File webrtc/api/video/i420_buffer.cc (right):

https://codereview.webrtc.org/2906053002/diff/40001/webrtc/api/video/i420_buf...
webrtc/api/video/i420_buffer.cc:108: RTC_CHECK(src.type() == Type::kI420);
On 2017/05/29 12:37:17, nisse-webrtc wrote:
> On 2017/05/29 12:12:06, magjed_webrtc wrote:
> > On 2017/05/29 08:22:22, nisse-webrtc wrote:
> > > Hmm. To get better compile time type-checking, could we add some abstract
> > > classes for I420 and I444 buffers, which would inherit PlanarYuvBuffer and
> > > define the type() method only (and possibly define it using |final|).
> 
> > Yeah, this is something I thought about when I did the first CL for exactly
> the
> > reasons you mention. I think it might be inflexible though to have a class
for
> > 'type() == Type::kI420', something that is currently dynamic.
> 
> Maybe we have to prepare some experimental cl to find out if it's feasible?
> 
> > For example, when
> > implementing WrappedPlanarYuvBuffer, we can now send the type as an argument
> and
> > only need one class. If we make separate types for I420 and I444 we would
need
> > two classes WrappedI420 and WrappedI444 in order to return different types,
> 
> I guess one could avoid most code duplication by making the base type be a
> template argument, but it might still be a bit awkward.
> 
> > I'm still on the fence, but if we want to change it, we should do it asap.
> What
> > do you think?
> 
> I think proper type checking for functions which require input arguments to be
> I420 is a pretty strong reasong for doing this change. And it's hard to judge
> the drawbacks without writing the code.
> 
> And we also have the hard naming problem... I420Buffer is taken, for the
> implementation class. Should it be I420BufferInterface? That's not very
pretty.

It's feasible, I uploaded a CL here: https://codereview.webrtc.org/2914463002/.
I had to go with the name I420BufferInterface. Maybe we could change I420Buffer
to I420BufferImpl and I420BufferInterface to I420Buffer in the future, but the
names are not a big issue for me.

https://codereview.webrtc.org/2906053002/diff/40001/webrtc/media/base/adapted...
File webrtc/media/base/adaptedvideotracksource.cc (right):

https://codereview.webrtc.org/2906053002/diff/40001/webrtc/media/base/adapted...
webrtc/media/base/adaptedvideotracksource.cc:52:
webrtc::I420Buffer::Rotate(*buffer->GetI420(), frame.rotation()),
On 2017/05/29 12:37:17, nisse-webrtc wrote:
> On 2017/05/29 12:12:06, magjed_webrtc wrote:
> > On 2017/05/29 08:22:22, nisse-webrtc wrote:
> > > What's the right way to add support for rotating non-I420 frames here? It
> > would
> > > be nice to not have a |switch(buffer->type())|.
> > 
> > Hopefully we don't need to handle rotating non-I420 frames. Rotating buffers
> is
> > something we want to avoid, and it's fine for me to always use ToI420() when
> > apply_rotation() is true. In general, I prefer to switch on the type instead
> of
> > adding more virtual functions to VideoFrameBuffer.
> 
> Hmm, in the case of texture frames rotation mismatch here is always temporary
> and we prefer dropping a frame or two over suddenly inserting a non-texture
> frame into the pipeline. I guess we need the same to hold for I444 frames, or
> else we'll pass I444 or I420 downstream depending on the current orientation.
> 
> Not sure what code depends on apply_rotation being handled here.

Only the Android capture implementation depends on it. We could move this logic
there if you prefer.

[nisse-webrtc, 2017-05-29 14:35:55]

https://codereview.webrtc.org/2906053002/diff/40001/webrtc/api/video/i420_buf...
File webrtc/api/video/i420_buffer.cc (right):

https://codereview.webrtc.org/2906053002/diff/40001/webrtc/api/video/i420_buf...
webrtc/api/video/i420_buffer.cc:108: RTC_CHECK(src.type() == Type::kI420);
On 2017/05/29 13:57:02, magjed_webrtc wrote:
> On 2017/05/29 12:37:17, nisse-webrtc wrote:
> > On 2017/05/29 12:12:06, magjed_webrtc wrote:
> > > On 2017/05/29 08:22:22, nisse-webrtc wrote:
> > > > Hmm. To get better compile time type-checking, could we add some
abstract
> > > > classes for I420 and I444 buffers, which would inherit PlanarYuvBuffer
and
> > > > define the type() method only (and possibly define it using |final|).
> > 
> > > Yeah, this is something I thought about when I did the first CL for
exactly
> > the
> > > reasons you mention. I think it might be inflexible though to have a class
> for
> > > 'type() == Type::kI420', something that is currently dynamic.
> > 
> > Maybe we have to prepare some experimental cl to find out if it's feasible?
> > 
> > > For example, when
> > > implementing WrappedPlanarYuvBuffer, we can now send the type as an
argument
> > and
> > > only need one class. If we make separate types for I420 and I444 we would
> need
> > > two classes WrappedI420 and WrappedI444 in order to return different
types,
> > 
> > I guess one could avoid most code duplication by making the base type be a
> > template argument, but it might still be a bit awkward.
> > 
> > > I'm still on the fence, but if we want to change it, we should do it asap.
> > What
> > > do you think?
> > 
> > I think proper type checking for functions which require input arguments to
be
> > I420 is a pretty strong reasong for doing this change. And it's hard to
judge
> > the drawbacks without writing the code.
> > 
> > And we also have the hard naming problem... I420Buffer is taken, for the
> > implementation class. Should it be I420BufferInterface? That's not very
> pretty.
> 
> It's feasible, I uploaded a CL here:
https://codereview.webrtc.org/2914463002/.
> I had to go with the name I420BufferInterface. Maybe we could change
I420Buffer
> to I420BufferImpl and I420BufferInterface to I420Buffer in the future, but the
> names are not a big issue for me.

I420BufferInterface is good enough, I think. And I'm not so fond of the *Impl
naming convention either.

https://codereview.webrtc.org/2906053002/diff/40001/webrtc/media/base/adapted...
File webrtc/media/base/adaptedvideotracksource.cc (right):

https://codereview.webrtc.org/2906053002/diff/40001/webrtc/media/base/adapted...
webrtc/media/base/adaptedvideotracksource.cc:52:
webrtc::I420Buffer::Rotate(*buffer->GetI420(), frame.rotation()),
On 2017/05/29 13:57:02, magjed_webrtc wrote:
> On 2017/05/29 12:37:17, nisse-webrtc wrote:
> > On 2017/05/29 12:12:06, magjed_webrtc wrote:
> > > On 2017/05/29 08:22:22, nisse-webrtc wrote:
> > > > What's the right way to add support for rotating non-I420 frames here?
It
> > > would
> > > > be nice to not have a |switch(buffer->type())|.
> > > 
> > > Hopefully we don't need to handle rotating non-I420 frames. Rotating
buffers
> > is
> > > something we want to avoid, and it's fine for me to always use ToI420()
when
> > > apply_rotation() is true. In general, I prefer to switch on the type
instead
> > of
> > > adding more virtual functions to VideoFrameBuffer.
> > 
> > Hmm, in the case of texture frames rotation mismatch here is always
temporary
> > and we prefer dropping a frame or two over suddenly inserting a non-texture
> > frame into the pipeline. I guess we need the same to hold for I444 frames,
or
> > else we'll pass I444 or I420 downstream depending on the current
orientation.
> > 
> > Not sure what code depends on apply_rotation being handled here.
> 
> Only the Android capture implementation depends on it. We could move this
logic
> there if you prefer.

I think logic which is generally useful to any video source belongs in this
class. If almost all sources do their own rotation, then it makes sense to
delete the code here (but keep the apply_rotation() method).

Failure mode is like this, I think: If a source doesn't implement apply rotation
properly, and some sink sets wants.rotation_applied, then that sink will not get
any frames, by the logic in VideoBroadcaster::OnFrame.

[magjed_webrtc, 2017-05-30 09:44:12]

The CQ bit was checked by magjed@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-30 09:44:16]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-05-30 10:11:14]

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

[commit-bot: I haz the power, 2017-05-30 10:11:15]

Dry run: This issue passed the CQ dry run.

[magjed_webrtc, 2017-05-30 11:04:46]

nisse - I have rebased against the other CL, please take another look

[nisse-webrtc, 2017-05-30 12:35:40]

Still looks pretty good. Consider making the Get* accessors use const. (Could be
a separate cl, if it's a large change).

https://codereview.webrtc.org/2906053002/diff/60001/webrtc/api/video/i420_buf...
File webrtc/api/video/i420_buffer.h (right):

https://codereview.webrtc.org/2906053002/diff/60001/webrtc/api/video/i420_buf...
webrtc/api/video/i420_buffer.h:36: return
Copy(*const_cast<VideoFrameBuffer&>(buffer).GetI420());
Any reason we can't mark the GetI420 and GetI44 methods const? Then you wouldn't
need this const_cast (and the one below).

Hmm, they'd need to return scoped_refptr<const I420BufferInterface> then (or a
raw const ptr), is that a problem? Ref counts are mutable, I think.

Ideally, we shouldn't be using any non-const references using these interfaces;
frame buffers should only be modified close to creation, using a more specific
type like I420Buffer. 

But current code doesn't look like that, unfortunately.
If needed, we could have one const and one non-const version of GetI420 and
GetI444.

[magjed_webrtc, 2017-05-30 13:18:09]

https://codereview.webrtc.org/2906053002/diff/60001/webrtc/api/video/i420_buf...
File webrtc/api/video/i420_buffer.h (right):

https://codereview.webrtc.org/2906053002/diff/60001/webrtc/api/video/i420_buf...
webrtc/api/video/i420_buffer.h:36: return
Copy(*const_cast<VideoFrameBuffer&>(buffer).GetI420());
On 2017/05/30 12:35:40, nisse-webrtc wrote:
> Any reason we can't mark the GetI420 and GetI44 methods const? Then you
wouldn't
> need this const_cast (and the one below).
> 
> Hmm, they'd need to return scoped_refptr<const I420BufferInterface> then (or a
> raw const ptr), is that a problem? Ref counts are mutable, I think.
> 
> Ideally, we shouldn't be using any non-const references using these
interfaces;
> frame buffers should only be modified close to creation, using a more specific
> type like I420Buffer. 
> 
> But current code doesn't look like that, unfortunately.
> If needed, we could have one const and one non-const version of GetI420 and
> GetI444.

Yes, the problem is the return type... I would love to have the Get functions
const, but I don't want to return a scoped_refptr<const I420BufferInterface>
since that would cause a lot of trouble with everything that wants a
scoped_refptr<I420BufferInterface>. I would be ok with returning a raw const
pointer from the Get functions like we discussed, but I need to remove the
I420Wrapper class in video_frame_buffer.cc first. I think we need to do this
incrementally, so how about we land this CL as is and clean up the Get functions
later? I hate const_cast, but hopefully I can remove these temporary functions
quickly.

[nisse-webrtc, 2017-05-30 13:28:22]

https://codereview.webrtc.org/2906053002/diff/60001/webrtc/api/video/i420_buf...
File webrtc/api/video/i420_buffer.h (right):

https://codereview.webrtc.org/2906053002/diff/60001/webrtc/api/video/i420_buf...
webrtc/api/video/i420_buffer.h:36: return
Copy(*const_cast<VideoFrameBuffer&>(buffer).GetI420());
On 2017/05/30 13:18:08, magjed_webrtc wrote:
> Yes, the problem is the return type... I would love to have the Get functions
> const, but I don't want to return a scoped_refptr<const I420BufferInterface>
> since that would cause a lot of trouble with everything that wants a
> scoped_refptr<I420BufferInterface>.

Is that a lot of places? Current version of this cl replaces
scoped_refptr<VideoFrameBuffer> by scoped_refptr<I420BufferInterface> in lots of
places, I was hoping it would be easy to change to scoped_refptr<const
I420bufferInterface> instead.

But if it's a lot of work, we can postpone it. Just add a TODO item on the
declarations of these methods saying they ought to be const (and possibly raw
pointers?), and briefly explaining why they aren't.

[magjed_webrtc, 2017-05-30 14:00:56]

The CQ bit was checked by magjed@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-30 14:00:58]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[magjed_webrtc, 2017-05-30 14:25:21]

https://codereview.webrtc.org/2906053002/diff/60001/webrtc/api/video/i420_buf...
File webrtc/api/video/i420_buffer.h (right):

https://codereview.webrtc.org/2906053002/diff/60001/webrtc/api/video/i420_buf...
webrtc/api/video/i420_buffer.h:36: return
Copy(*const_cast<VideoFrameBuffer&>(buffer).GetI420());
On 2017/05/30 13:28:21, nisse-webrtc wrote:
> On 2017/05/30 13:18:08, magjed_webrtc wrote:
> > Yes, the problem is the return type... I would love to have the Get
functions
> > const, but I don't want to return a scoped_refptr<const I420BufferInterface>
> > since that would cause a lot of trouble with everything that wants a
> > scoped_refptr<I420BufferInterface>.
> 
> Is that a lot of places? Current version of this cl replaces
> scoped_refptr<VideoFrameBuffer> by scoped_refptr<I420BufferInterface> in lots
of
> places, I was hoping it would be easy to change to scoped_refptr<const
> I420bufferInterface> instead.
> 
> But if it's a lot of work, we can postpone it. Just add a TODO item on the
> declarations of these methods saying they ought to be const (and possibly raw
> pointers?), and briefly explaining why they aren't. 

Not many places it seems like, done.

[commit-bot: I haz the power, 2017-05-30 14:28:57]

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

[commit-bot: I haz the power, 2017-05-30 14:28:58]

Dry run: This issue passed the CQ dry run.

[nisse-webrtc, 2017-05-30 14:59:31]

lgtm, great!

[magjed_webrtc, 2017-05-31 08:37:29]

magjed@webrtc.org changed reviewers:
+ stefan@webrtc.org

[magjed_webrtc, 2017-05-31 08:38:01]

stefan - please review webrtc/api/video/

[magjed_webrtc, 2017-06-01 16:24:37]

Description was changed from

==========
Update I420Buffer to new VideoFrameBuffer interface

This is a follow-up cleanup for CL https://codereview.webrtc.org/2847383002/.

BUG=webrtc:7632
==========

to

==========
Update I420Buffer to new VideoFrameBuffer interface

This is a follow-up cleanup for CL https://codereview.webrtc.org/2847383002/.

BUG=webrtc:7632
TBR=stefan
==========

[magjed_webrtc, 2017-06-01 16:26:59]

Stefan - I'm TBR:ing you for the changes in webrtc/api/video/, they are quite
simple. Review if you are interested and have time.

[magjed_webrtc, 2017-06-01 16:27:18]

The CQ bit was checked by magjed@webrtc.org

[commit-bot: I haz the power, 2017-06-01 16:27:39]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-06-01 17:02:27]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1496334438000170,
"parent_rev": "c757293f15c60080bdabf1a14b65e8ba1c2af4a3", "commit_rev":
"3f075498a3a3d7426deded720e21a6f77d1cabef"}

[commit-bot: I haz the power, 2017-06-01 17:02:31]

Description was changed from

==========
Update I420Buffer to new VideoFrameBuffer interface

This is a follow-up cleanup for CL https://codereview.webrtc.org/2847383002/.

BUG=webrtc:7632
TBR=stefan
==========

to

==========
Update I420Buffer to new VideoFrameBuffer interface

This is a follow-up cleanup for CL https://codereview.webrtc.org/2847383002/.

BUG=webrtc:7632
TBR=stefan

Review-Url: https://codereview.webrtc.org/2906053002
Cr-Commit-Position: refs/heads/master@{#18388}
Committed:
https://chromium.googlesource.com/external/webrtc/+/3f075498a3a3d7426deded720...
==========

[commit-bot: I haz the power, 2017-06-01 17:02:32]

Committed patchset #3 (id:80001) as
https://chromium.googlesource.com/external/webrtc/+/3f075498a3a3d7426deded720...