Add separate base classes for I420 and I444 buffers

Add separate base classes for I420 and I444 buffers

Previously, the base class PlanarYuvBuffer was used directly. Having
separate base classes will allow us to improve type safety in some
places.

BUG=webrtc:7632
TBR=stefan@webrtc.org

Review-Url: https://codereview.webrtc.org/2914463002
Cr-Commit-Position: refs/heads/master@{#18317}
Committed: https://chromium.googlesource.com/external/webrtc/+/eaf4a1e10372e1a5a09f8669fca6ae61b156837f

[magjed_webrtc, 2017-05-29 13:07:00]

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

[commit-bot: I haz the power, 2017-05-29 13:07:06]

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-29 13:28:04]

Description was changed from

==========
Add separate base classes for I420 and I444 buffers

Previously, the base class PlanarYuvBuffer was used directly. Having
separate base classes will allow us to improve type safety in some
places.

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

to

==========
Add separate base classes for I420 and I444 buffers

Previously, the base class PlanarYuvBuffer was used directly. Having
separate base classes will allow us to improve type safety in some
places.

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

[magjed_webrtc, 2017-05-29 13:28:04]

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

[magjed_webrtc, 2017-05-29 13:28:14]

nisse - how about this?

[commit-bot: I haz the power, 2017-05-29 13:31:36]

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

[commit-bot: I haz the power, 2017-05-29 13:31:36]

Dry run: This issue passed the CQ dry run.

[nisse-webrtc, 2017-05-29 14:20:05]

Looks pretty good. I'd prefer the GetI420 and GetI444 methods to be virtual,
replacing a conditional by vtable lookup. Similarly to the ChromaWidth and
ChromaHeight methods.

https://codereview.webrtc.org/2914463002/diff/1/webrtc/api/video/video_frame_...
File webrtc/api/video/video_frame_buffer.cc (right):

https://codereview.webrtc.org/2914463002/diff/1/webrtc/api/video/video_frame_...
webrtc/api/video/video_frame_buffer.cc:94:
rtc::scoped_refptr<I420BufferInterface> VideoFrameBuffer::GetI420() {
I think this default implementation can be deleted when the I420InterfaceAdapter
is deleted.

https://codereview.webrtc.org/2914463002/diff/1/webrtc/api/video/video_frame_...
File webrtc/api/video/video_frame_buffer.h (right):

https://codereview.webrtc.org/2914463002/diff/1/webrtc/api/video/video_frame_...
webrtc/api/video/video_frame_buffer.h:66:
rtc::scoped_refptr<I420BufferInterface> GetI420();
I think it's nicer if we make these functions virtual, possibly with default
implementations using RTC_NOTREACHED. And then override appropriately in the
I420 and I444 subclasses, without any explicit casts.

Unrelated to this cl: Do these methods really need to return smart pointers?
Depends a bit on how they're used, but they will never do anything interesting
with ownership. 

And if we move to std::shared_ptr (with a refcount outside of the object itself)
it can no longer return a smart pointer, as far as I understand, which seems
like a tricky problem for methods like ToI420 which sometimes want to return a
reference to |this|, and sometimes a reference to a newly constructed object.

https://codereview.webrtc.org/2914463002/diff/1/webrtc/api/video/video_frame_...
webrtc/api/video/video_frame_buffer.h:121:
rtc::scoped_refptr<I420BufferInterface> ToI420() final;
Here we could also put the GetI420() final {return this;}

[magjed_webrtc, 2017-05-29 17:40:07]

https://codereview.webrtc.org/2914463002/diff/1/webrtc/api/video/video_frame_...
File webrtc/api/video/video_frame_buffer.h (right):

https://codereview.webrtc.org/2914463002/diff/1/webrtc/api/video/video_frame_...
webrtc/api/video/video_frame_buffer.h:66:
rtc::scoped_refptr<I420BufferInterface> GetI420();
On 2017/05/29 14:20:05, nisse-webrtc wrote:
> I think it's nicer if we make these functions virtual, possibly with default
> implementations using RTC_NOTREACHED. And then override appropriately in the
> I420 and I444 subclasses, without any explicit casts.
> 
> Unrelated to this cl: Do these methods really need to return smart pointers?
> Depends a bit on how they're used, but they will never do anything interesting
> with ownership. 
> 
> And if we move to std::shared_ptr (with a refcount outside of the object
itself)
> it can no longer return a smart pointer, as far as I understand, which seems
> like a tricky problem for methods like ToI420 which sometimes want to return a
> reference to |this|, and sometimes a reference to a newly constructed object.

What benefit do you see from making these functions virtual? IMO, it will
complicate the interface. Do you have any practical use case in mind that
requires virtual functions?

Regarding returning smart pointers. It's true that it would be possible to
return raw pointers, but I prefer to not expose raw pointers of ref counted
objects.

[nisse-webrtc, 2017-05-30 07:05:33]

lgtm. Still prefer virtual, but it's not essential.

https://codereview.webrtc.org/2914463002/diff/1/webrtc/api/video/video_frame_...
File webrtc/api/video/video_frame_buffer.h (right):

https://codereview.webrtc.org/2914463002/diff/1/webrtc/api/video/video_frame_...
webrtc/api/video/video_frame_buffer.h:66:
rtc::scoped_refptr<I420BufferInterface> GetI420();
On 2017/05/29 17:40:07, magjed_webrtc wrote:
 
> What benefit do you see from making these functions virtual? IMO, it will
> complicate the interface. Do you have any practical use case in mind that
> requires virtual functions?

I see two (weak) benefits: Consistency with the other methods whose
implementation depend on the type, i.e., ChromaWidth() and ChromaHeight(), which
are virtual. And we get rid of two explicit casts. But it's partly a question of
taste, where I prefer dispatch via the vtable over explicit run-time type
checks.

> Regarding returning smart pointers. It's true that it would be possible to
> return raw pointers, but I prefer to not expose raw pointers of ref counted
> objects.

I see. Style-guide isn't crystal clear, "return raw pointers when the caller
does not take ownership, and return smart pointers by value otherwise"
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md 

In this case, the caller already has ownership, but returning a smart pointer
enables the caller to destroy and forget about the the original reference.

I think we can leave as is and revisit if/when we want to move over to
std::shared_ptr.

[kwiberg-webrtc, 2017-05-30 07:32:07]

https://codereview.webrtc.org/2914463002/diff/1/webrtc/api/video/video_frame_...
File webrtc/api/video/video_frame_buffer.h (right):

https://codereview.webrtc.org/2914463002/diff/1/webrtc/api/video/video_frame_...
webrtc/api/video/video_frame_buffer.h:66:
rtc::scoped_refptr<I420BufferInterface> GetI420();
On 2017/05/30 07:05:32, nisse-webrtc wrote:
> On 2017/05/29 17:40:07, magjed_webrtc wrote:
>  
> > What benefit do you see from making these functions virtual? IMO, it will
> > complicate the interface. Do you have any practical use case in mind that
> > requires virtual functions?
> 
> I see two (weak) benefits: Consistency with the other methods whose
> implementation depend on the type, i.e., ChromaWidth() and ChromaHeight(),
which
> are virtual. And we get rid of two explicit casts. But it's partly a question
of
> taste, where I prefer dispatch via the vtable over explicit run-time type
> checks.
> 
> > Regarding returning smart pointers. It's true that it would be possible to
> > return raw pointers, but I prefer to not expose raw pointers of ref counted
> > objects.
> 
> I see. Style-guide isn't crystal clear, "return raw pointers when the caller
> does not take ownership, and return smart pointers by value otherwise"
> https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md 
> 
> In this case, the caller already has ownership, but returning a smart pointer
> enables the caller to destroy and forget about the the original reference.
> 
> I think we can leave as is and revisit if/when we want to move over to
> std::shared_ptr.

Drive-by opinion: Don't be afraid of raw pointers to refcounted objects. A raw
pointer simply means that you point to something you don't own, and that you
shouldn't make assumptions about how its lifetime is managed.

(We have historically allowed conversion from T* to scoped_refptr<T>, but that's
a bad idea and no one should be doing it.)

[magjed_webrtc, 2017-05-30 07:49:02]

The CQ bit was checked by magjed@webrtc.org

[commit-bot: I haz the power, 2017-05-30 07:49:10]

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 07:52:53]

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

[commit-bot: I haz the power, 2017-05-30 07:52:53]

Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/17526)

[magjed_webrtc, 2017-05-30 08:19:34]

Description was changed from

==========
Add separate base classes for I420 and I444 buffers

Previously, the base class PlanarYuvBuffer was used directly. Having
separate base classes will allow us to improve type safety in some
places.

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

to

==========
Add separate base classes for I420 and I444 buffers

Previously, the base class PlanarYuvBuffer was used directly. Having
separate base classes will allow us to improve type safety in some
places.

BUG=webrtc:7632
TBR=stefan@webrtc.org
==========

[magjed_webrtc, 2017-05-30 08:19:34]

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

[magjed_webrtc, 2017-05-30 08:19:42]

The CQ bit was checked by magjed@webrtc.org

[commit-bot: I haz the power, 2017-05-30 08:19:47]

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 08:21:59]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1496132382826440, "parent_rev":
"7b1b13b2fa1eeea718c120fe70e6fac5c1b56677", "commit_rev":
"eaf4a1e10372e1a5a09f8669fca6ae61b156837f"}

[commit-bot: I haz the power, 2017-05-30 08:22:02]

Description was changed from

==========
Add separate base classes for I420 and I444 buffers

Previously, the base class PlanarYuvBuffer was used directly. Having
separate base classes will allow us to improve type safety in some
places.

BUG=webrtc:7632
TBR=stefan@webrtc.org
==========

to

==========
Add separate base classes for I420 and I444 buffers

Previously, the base class PlanarYuvBuffer was used directly. Having
separate base classes will allow us to improve type safety in some
places.

BUG=webrtc:7632
TBR=stefan@webrtc.org

Review-Url: https://codereview.webrtc.org/2914463002
Cr-Commit-Position: refs/heads/master@{#18317}
Committed:
https://chromium.googlesource.com/external/webrtc/+/eaf4a1e10372e1a5a09f8669f...
==========

[commit-bot: I haz the power, 2017-05-30 08:22:03]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/external/webrtc/+/eaf4a1e10372e1a5a09f8669f...