Delete IsMutable and IsExclusive methods.

Delete IsMutable and IsExclusive methods.

This affects the webrtc::VideoFrameBuffer and cricket::VideoFrame
classes.

To make this work, VideoFrameFactory is changed to use an
I420BufferPool rather than a plain VideoFrame to cache allocated
frames.

The I420BufferPool is reorganized to return an I420Buffer,
rather than a proxy object.

BUG=webrtc:5921, webrtc:5682
Committed: https://crrev.com/d591e3fcf33a4c692cd49294b22dd21bb117cd19
Cr-Commit-Position: refs/heads/master@{#12919}

[nisse-webrtc, 2016-05-25 10:21:13]

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

[commit-bot: I haz the power, 2016-05-25 10:21:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2009193002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2009193002/1

[commit-bot: I haz the power, 2016-05-25 10:28:51]

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

[commit-bot: I haz the power, 2016-05-25 10:28:52]

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

[nisse-webrtc, 2016-05-25 10:58:09]

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

[commit-bot: I haz the power, 2016-05-25 10:58:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2009193002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2009193002/20001

[nisse-webrtc, 2016-05-25 11:42:14]

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

[nisse-webrtc, 2016-05-25 11:42:15]

This cl deletes IsMutable, and reorganizes I420BufferPool.

The messy part was the VideoFrameFactory. To make it use a frame pool, somewhat
ugly changes to cricket::VideoFrame were needed to be able to call Stretch*
using a framebuffer from a frame pool. May be some prettier way to do it, some
methods would make more sense in the FrameFactory interface rather than the
VideoFrame interface. But hopefully we'll get rid of the frame factories in the
not too distant future.

[nisse-webrtc, 2016-05-25 11:43:49]

On 2016/05/25 11:42:15, nisse-webrtc wrote:
> This cl deletes IsMutable, and reorganizes I420BufferPool.
> 
> The messy part was the VideoFrameFactory. To make it use a frame pool,
somewhat
> ugly changes to cricket::VideoFrame were needed to be able to call Stretch*
> using a framebuffer from a frame pool. May be some prettier way to do it, some
> methods would make more sense in the FrameFactory interface rather than the
> VideoFrame interface. But hopefully we'll get rid of the frame factories in
the
> not too distant future.

I'm not sure if it's practical to move the MutableData methods in the same cl,
doing that is more likely to break downstream users than this cl. I'll
investigate a bit.

[tommi, 2016-05-25 12:14:24]

tommi@webrtc.org changed reviewers:
+ tommi@webrtc.org

[tommi, 2016-05-25 12:14:25]

https://codereview.webrtc.org/2009193002/diff/20001/webrtc/media/base/videofr...
File webrtc/media/base/videoframe.h (right):

https://codereview.webrtc.org/2009193002/diff/20001/webrtc/media/base/videofr...
webrtc/media/base/videoframe.h:63: const
rtc::scoped_refptr<webrtc::VideoFrameBuffer> buffer,
const &?

https://codereview.webrtc.org/2009193002/diff/20001/webrtc/media/base/videofr...
webrtc/media/base/videoframe.h:102: #if 0
delete this method?

https://codereview.webrtc.org/2009193002/diff/20001/webrtc/media/base/videofr...
File webrtc/media/base/videoframefactory.cc (right):

https://codereview.webrtc.org/2009193002/diff/20001/webrtc/media/base/videofr...
webrtc/media/base/videoframefactory.cc:49: return output_frame.release();
could we change the return value of CreateAliasedFrame to be
std::unique_ptr<VideoFrame>?

https://codereview.webrtc.org/2009193002/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoframe.cc (right):

https://codereview.webrtc.org/2009193002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoframe.cc:87: VideoFrame*
WebRtcVideoFrame::CreateWithBuffer(
return std::unique_ptr<VideoFrame>?

[commit-bot: I haz the power, 2016-05-25 12:36:23]

Dry run: None

[nisse-webrtc, 2016-05-25 12:38:02]

Updated, deleted the old Stretch method, and changed the signature of
CreateWithBuffer.

No change of the CreateAliasedFrame return type.

https://codereview.webrtc.org/2009193002/diff/20001/webrtc/media/base/videofr...
File webrtc/media/base/videoframe.h (right):

https://codereview.webrtc.org/2009193002/diff/20001/webrtc/media/base/videofr...
webrtc/media/base/videoframe.h:63: const
rtc::scoped_refptr<webrtc::VideoFrameBuffer> buffer,
On 2016/05/25 12:14:25, tommi-webrtc wrote:
> const &?

Done.

https://codereview.webrtc.org/2009193002/diff/20001/webrtc/media/base/videofr...
webrtc/media/base/videoframe.h:102: #if 0
On 2016/05/25 12:14:25, tommi-webrtc wrote:
> delete this method?

Sorry for this left-over. It is no longer used, and now deleted.

https://codereview.webrtc.org/2009193002/diff/20001/webrtc/media/base/videofr...
File webrtc/media/base/videoframefactory.cc (right):

https://codereview.webrtc.org/2009193002/diff/20001/webrtc/media/base/videofr...
webrtc/media/base/videoframefactory.cc:49: return output_frame.release();
On 2016/05/25 12:14:25, tommi-webrtc wrote:
> could we change the return value of CreateAliasedFrame to be
> std::unique_ptr<VideoFrame>?

Not easily. There's still a VideoFrameFactory subclass in chrome, which would
break.

https://codereview.webrtc.org/2009193002/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoframe.cc (right):

https://codereview.webrtc.org/2009193002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoframe.cc:87: VideoFrame*
WebRtcVideoFrame::CreateWithBuffer(
On 2016/05/25 12:14:25, tommi-webrtc wrote:
> return std::unique_ptr<VideoFrame>?

That's inconsistent with neighboring methods, but I guess it still makes sense.
Done.

[tommi, 2016-05-25 12:39:56]

thanks. lgtm

[magjed_webrtc, 2016-05-25 13:17:01]

https://codereview.webrtc.org/2009193002/diff/40001/webrtc/common_video/inclu...
File webrtc/common_video/include/i420_buffer_pool.h (right):

https://codereview.webrtc.org/2009193002/diff/40001/webrtc/common_video/inclu...
webrtc/common_video/include/i420_buffer_pool.h:39: class PooledI420Buffer :
public webrtc::I420Buffer {
I don't see the need for this class, why not just use I420Buffer directly?

https://codereview.webrtc.org/2009193002/diff/40001/webrtc/media/base/videofr...
File webrtc/media/base/videoframe.h (right):

https://codereview.webrtc.org/2009193002/diff/40001/webrtc/media/base/videofr...
webrtc/media/base/videoframe.h:60: // TODO(nisse): Needed by VideoFactory. Must
be deleted for merge of
I don't see the need for this function. Why not use the ctor of WebRtcVideoFrame
directly?

https://codereview.webrtc.org/2009193002/diff/40001/webrtc/media/base/videofr...
File webrtc/media/base/videoframefactory.cc (right):

https://codereview.webrtc.org/2009193002/diff/40001/webrtc/media/base/videofr...
webrtc/media/base/videoframefactory.cc:42: std::unique_ptr<VideoFrame>
output_frame(
Why CreateWithBuffer? Why not just:
new WebRtcVideoFrame(pool_.CreateBuffer(output_width, output_height),
cropped_input_frame->rotation(), cropped_input_frame->timestamp_us());

[nisse-webrtc, 2016-05-25 13:39:47]

https://codereview.webrtc.org/2009193002/diff/40001/webrtc/common_video/inclu...
File webrtc/common_video/include/i420_buffer_pool.h (right):

https://codereview.webrtc.org/2009193002/diff/40001/webrtc/common_video/inclu...
webrtc/common_video/include/i420_buffer_pool.h:39: class PooledI420Buffer :
public webrtc::I420Buffer {
On 2016/05/25 13:17:01, magjed_webrtc wrote:
> I don't see the need for this class, why not just use I420Buffer directly?

To use I420Buffer directly, I have to include HasOneRef in it's public
interface. Ok?

https://codereview.webrtc.org/2009193002/diff/40001/webrtc/media/base/videofr...
File webrtc/media/base/videoframe.h (right):

https://codereview.webrtc.org/2009193002/diff/40001/webrtc/media/base/videofr...
webrtc/media/base/videoframe.h:60: // TODO(nisse): Needed by VideoFactory. Must
be deleted for merge of
On 2016/05/25 13:17:01, magjed_webrtc wrote:
> I don't see the need for this function. Why not use the ctor of
WebRtcVideoFrame
> directly?

It's used in VideoFrameFactory::CreateAliasedFrame, i.e., the base class. Which
is supposed to be unaware of the WebRtcVideoFrame class, right?

I could move it to a virtual VideoFrameFactory method, and implement it in
WebRtcVideoFrameFactory, which should know about WebRtcVideoFrame. But I guess
that would have a higher probability of breaking chrome.

I dislike the frame factories, so I'd prefer to do as small changes as possible,
until I get the chance to delete them.

https://codereview.webrtc.org/2009193002/diff/40001/webrtc/media/base/videofr...
File webrtc/media/base/videoframefactory.cc (right):

https://codereview.webrtc.org/2009193002/diff/40001/webrtc/media/base/videofr...
webrtc/media/base/videoframefactory.cc:42: std::unique_ptr<VideoFrame>
output_frame(
On 2016/05/25 13:17:01, magjed_webrtc wrote:
> Why CreateWithBuffer? Why not just:
> new WebRtcVideoFrame(pool_.CreateBuffer(output_width, output_height),
> cropped_input_frame->rotation(), cropped_input_frame->timestamp_us());

My understanding is that this code is supposed to not know what type of frames
it really is handling, but nevertheless produce a new frame of the same class as
cropped_input_frame, which was produced by a subclass's implementation of
CreateAliasedFrame(3 args).

So using the WebRtcVideoFrame constructor doesn't seem right.

It uses a pool (previously a single frame and IsExclusive) for the scaled frame,
but WebRtcVideoFrameFactory doesn't use any pool for the unscaled, likely
larger, frame. So it makes little sense.

The factory-less android code in my other cl uses two pools, used before and
after scaling. Which  looks more like the right thing, if we don't want large
allocations for each incoming frame.

[magjed_webrtc, 2016-05-25 14:56:08]

https://codereview.webrtc.org/2009193002/diff/40001/webrtc/common_video/inclu...
File webrtc/common_video/include/i420_buffer_pool.h (right):

https://codereview.webrtc.org/2009193002/diff/40001/webrtc/common_video/inclu...
webrtc/common_video/include/i420_buffer_pool.h:39: class PooledI420Buffer :
public webrtc::I420Buffer {
On 2016/05/25 13:39:47, nisse-webrtc wrote:
> On 2016/05/25 13:17:01, magjed_webrtc wrote:
> > I don't see the need for this class, why not just use I420Buffer directly?
> 
> To use I420Buffer directly, I have to include HasOneRef in it's public
> interface. Ok?

HasOneRef is already part of the VideoFrameBuffer interface so you don't have to
do anything.

https://codereview.webrtc.org/2009193002/diff/40001/webrtc/media/base/videofr...
File webrtc/media/base/videoframe.h (right):

https://codereview.webrtc.org/2009193002/diff/40001/webrtc/media/base/videofr...
webrtc/media/base/videoframe.h:60: // TODO(nisse): Needed by VideoFactory. Must
be deleted for merge of
On 2016/05/25 13:39:47, nisse-webrtc wrote:
> On 2016/05/25 13:17:01, magjed_webrtc wrote:
> > I don't see the need for this function. Why not use the ctor of
> WebRtcVideoFrame
> > directly?
> 
> It's used in VideoFrameFactory::CreateAliasedFrame, i.e., the base class.
Which
> is supposed to be unaware of the WebRtcVideoFrame class, right?
No, we don't have to care about that. The only implementation of
cricket::VideoFrame is WebRtcVideoFrame and you are planning to remove them
anyway. So just use WebRtcVideoFrame directly, that's the simplest change.

https://codereview.webrtc.org/2009193002/diff/40001/webrtc/media/base/videofr...
File webrtc/media/base/videoframefactory.cc (right):

https://codereview.webrtc.org/2009193002/diff/40001/webrtc/media/base/videofr...
webrtc/media/base/videoframefactory.cc:42: std::unique_ptr<VideoFrame>
output_frame(
On 2016/05/25 13:39:47, nisse-webrtc wrote:
> On 2016/05/25 13:17:01, magjed_webrtc wrote:
> > Why CreateWithBuffer? Why not just:
> > new WebRtcVideoFrame(pool_.CreateBuffer(output_width, output_height),
> > cropped_input_frame->rotation(), cropped_input_frame->timestamp_us());
> 
> My understanding is that this code is supposed to not know what type of frames
> it really is handling, but nevertheless produce a new frame of the same class
as
> cropped_input_frame, which was produced by a subclass's implementation of
> CreateAliasedFrame(3 args).
It's not supposed to know anything about the type of frame except the interface,
but it's not required to return a frame of the same class. That's a strange
requirement, why would that be needed?

[pbos-webrtc, 2016-05-25 15:26:07]

https://codereview.webrtc.org/2009193002/diff/40001/webrtc/common_video/inclu...
File webrtc/common_video/include/i420_buffer_pool.h (right):

https://codereview.webrtc.org/2009193002/diff/40001/webrtc/common_video/inclu...
webrtc/common_video/include/i420_buffer_pool.h:39: class PooledI420Buffer :
public webrtc::I420Buffer {
On 2016/05/25 13:39:47, nisse-webrtc wrote:
> On 2016/05/25 13:17:01, magjed_webrtc wrote:
> > I don't see the need for this class, why not just use I420Buffer directly?
> 
> To use I420Buffer directly, I have to include HasOneRef in it's public
> interface. Ok?

Can you use a rtc::RefCountedObject<I420Buffer> ?

https://codereview.webrtc.org/2009193002/diff/40001/webrtc/common_video/video...
File webrtc/common_video/video_frame.cc (right):

https://codereview.webrtc.org/2009193002/diff/40001/webrtc/common_video/video...
webrtc/common_video/video_frame.cc:69: // Allocate a new buffer.
Should this check if video_frame_buffer_->HasOneRef() instead of IsMutable? Is
this method called often or going away?

[pbos-webrtc, 2016-05-25 15:26:38]

On 2016/05/25 15:26:07, pbos-webrtc wrote:
>
https://codereview.webrtc.org/2009193002/diff/40001/webrtc/common_video/inclu...
> File webrtc/common_video/include/i420_buffer_pool.h (right):
> 
>
https://codereview.webrtc.org/2009193002/diff/40001/webrtc/common_video/inclu...
> webrtc/common_video/include/i420_buffer_pool.h:39: class PooledI420Buffer :
> public webrtc::I420Buffer {
> On 2016/05/25 13:39:47, nisse-webrtc wrote:
> > On 2016/05/25 13:17:01, magjed_webrtc wrote:
> > > I don't see the need for this class, why not just use I420Buffer directly?
> > 
> > To use I420Buffer directly, I have to include HasOneRef in it's public
> > interface. Ok?
> 
> Can you use a rtc::RefCountedObject<I420Buffer> ?

Magnus' comments are probably better here. :)

>
https://codereview.webrtc.org/2009193002/diff/40001/webrtc/common_video/video...
> File webrtc/common_video/video_frame.cc (right):
> 
>
https://codereview.webrtc.org/2009193002/diff/40001/webrtc/common_video/video...
> webrtc/common_video/video_frame.cc:69: // Allocate a new buffer.
> Should this check if video_frame_buffer_->HasOneRef() instead of IsMutable? Is
> this method called often or going away?

[nisse-webrtc, 2016-05-26 07:44:43]

(No updated patchset now).

https://codereview.webrtc.org/2009193002/diff/40001/webrtc/common_video/inclu...
File webrtc/common_video/include/i420_buffer_pool.h (right):

https://codereview.webrtc.org/2009193002/diff/40001/webrtc/common_video/inclu...
webrtc/common_video/include/i420_buffer_pool.h:39: class PooledI420Buffer :
public webrtc::I420Buffer {
On 2016/05/25 14:56:07, magjed_webrtc wrote:
> On 2016/05/25 13:39:47, nisse-webrtc wrote:
> > On 2016/05/25 13:17:01, magjed_webrtc wrote:
> > > I don't see the need for this class, why not just use I420Buffer directly?
> > 
> > To use I420Buffer directly, I have to include HasOneRef in it's public
> > interface. Ok?
> 
> HasOneRef is already part of the VideoFrameBuffer interface so you don't have
to
> do anything.

This cl deletes it from the interface. I don't think it belongs there (and pbos
doesn't like it either, unless he has changed his mind since yesterday). A
public IsMutable (with a default implementation returning false) is more
reasonable, but we're deleting that method precisely because it's too tricky to
implement correctly for the buffer pool. 

If it's possible to let the buffer pool use the type
rtc::RefCountedObject<I420Buffer> (after all, that's the type it uses when
constructing new buffers, so in principle it should be easy to keep that type
and delay upcast until buffers are returned). But I'm not sure how to type the
corresponding scoped_refptr:s, I'd need a little help with that.

Otherwise, I'd prefer to keep the intermediate PooledI420Buffer class, for the
sole purpose to make HasOneRef accessible.

https://codereview.webrtc.org/2009193002/diff/40001/webrtc/common_video/video...
File webrtc/common_video/video_frame.cc (right):

https://codereview.webrtc.org/2009193002/diff/40001/webrtc/common_video/video...
webrtc/common_video/video_frame.cc:69: // Allocate a new buffer.
On 2016/05/25 15:26:07, pbos-webrtc wrote:
> Should this check if video_frame_buffer_->HasOneRef() instead of IsMutable? Is
> this method called often or going away?

It was used by the frame factory, that's why I changed it to make it use a
buffer pool instead. Besides that, the method is used only by test code.

And both HasOneRef and IsMutable are being purged from the public interface. (If
you want to keep one, IsMutable is the better name, imo).

https://codereview.webrtc.org/2009193002/diff/40001/webrtc/media/base/videofr...
File webrtc/media/base/videoframe.h (right):

https://codereview.webrtc.org/2009193002/diff/40001/webrtc/media/base/videofr...
webrtc/media/base/videoframe.h:60: // TODO(nisse): Needed by VideoFactory. Must
be deleted for merge of
On 2016/05/25 14:56:07, magjed_webrtc wrote:
> On 2016/05/25 13:39:47, nisse-webrtc wrote:
> > On 2016/05/25 13:17:01, magjed_webrtc wrote:
> > > I don't see the need for this function. Why not use the ctor of
> > WebRtcVideoFrame
> > > directly?
> > 
> > It's used in VideoFrameFactory::CreateAliasedFrame, i.e., the base class.
> Which
> > is supposed to be unaware of the WebRtcVideoFrame class, right?
> No, we don't have to care about that. The only implementation of
> cricket::VideoFrame is WebRtcVideoFrame and you are planning to remove them
> anyway. So just use WebRtcVideoFrame directly, that's the simplest change.

I see. I'll try that then.

https://codereview.webrtc.org/2009193002/diff/40001/webrtc/media/base/videofr...
File webrtc/media/base/videoframefactory.cc (right):

https://codereview.webrtc.org/2009193002/diff/40001/webrtc/media/base/videofr...
webrtc/media/base/videoframefactory.cc:42: std::unique_ptr<VideoFrame>
output_frame(
On 2016/05/25 14:56:07, magjed_webrtc wrote:
> On 2016/05/25 13:39:47, nisse-webrtc wrote:
> > On 2016/05/25 13:17:01, magjed_webrtc wrote:
> > > Why CreateWithBuffer? Why not just:
> > > new WebRtcVideoFrame(pool_.CreateBuffer(output_width, output_height),
> > > cropped_input_frame->rotation(), cropped_input_frame->timestamp_us());
> > 
> > My understanding is that this code is supposed to not know what type of
frames
> > it really is handling, but nevertheless produce a new frame of the same
class
> as
> > cropped_input_frame, which was produced by a subclass's implementation of
> > CreateAliasedFrame(3 args).
> It's not supposed to know anything about the type of frame except the
interface,
> but it's not required to return a frame of the same class. That's a strange
> requirement, why would that be needed?

I thought chrome might be picky. But it doesn't matter, it overrides both
CreateAliasedFrame, both 3-args and 5-args.

I'll drop CreateWithBuffer.

[nisse-webrtc, 2016-05-26 08:13:51]

Deleted the CreateWithBuffer method, calling the constructor directly.

Replaced the PooledI420Buffer subclass with a typedef (using) to
RefCountedObject<I420Buffer>. Which has the same effect, making HasOneRef
available to the pool.

[magjed_webrtc, 2016-05-26 11:34:32]

lgtm

[pbos-webrtc, 2016-05-26 11:40:07]

lgtm, nice!

[nisse-webrtc, 2016-05-26 11:59:34]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2016-05-26 11:59:35]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2009193002/#ps80001 (title:
"Delete redundant RefCountedObject wrapping.")

[commit-bot: I haz the power, 2016-05-26 11:59:44]

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

[commit-bot: I haz the power, 2016-05-26 13:50:00]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-26 13:50:07]

Description was changed from

==========
Delete IsMutable and IsExclusive methods.

This affects the webrtc::VideoFrameBuffer and cricket::VideoFrame
classes.

To make this work, VideoFrameFactory is changed to use an
I420BufferPool rather than a plain VideoFrame to cache allocated
frames.

The I420BufferPool is reorganized to return an I420Buffer,
rather than a proxy object.

BUG=webrtc:5921, webrtc:5682
==========

to

==========
Delete IsMutable and IsExclusive methods.

This affects the webrtc::VideoFrameBuffer and cricket::VideoFrame
classes.

To make this work, VideoFrameFactory is changed to use an
I420BufferPool rather than a plain VideoFrame to cache allocated
frames.

The I420BufferPool is reorganized to return an I420Buffer,
rather than a proxy object.

BUG=webrtc:5921, webrtc:5682

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

[commit-bot: I haz the power, 2016-05-26 13:50:08]

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