Add support for multiple pixel formats in VideoFrameBuffer

Add support for I444 in VideoFrameBuffer

VideoFrameBuffer is currently hard coded to be either I420 or Native.
This CL makes VideoFrameBuffer more generic by moving the I420 specific
functions into their own class, and adds an enum tag that represents the
format and storage type of the buffer. Each buffer type is then
represented as a subclass. See webrtc/api/video/video_frame_buffer.h for
more info.

This CL also adds support for representing I444 in VideoFrameBuffer
using the new interface. Possible future buffer type candidates are
RGB and NV12.

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

Review-Url: https://codereview.webrtc.org/2847383002
Cr-Commit-Position: refs/heads/master@{#18098}
Committed: https://chromium.googlesource.com/external/webrtc/+/712338eed24a8d68699b331acad12edf340ac929

[magjed_webrtc, 2017-04-29 12:30:28]

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

[commit-bot: I haz the power, 2017-04-29 12:30:35]

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-04-29 12:32:10]

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

[commit-bot: I haz the power, 2017-04-29 12:32:11]

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...)
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/19915)
  ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/19712)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/25226)
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/24097)

[magjed_webrtc, 2017-04-30 13:31:41]

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

[commit-bot: I haz the power, 2017-04-30 13:31:48]

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-04-30 13:36:18]

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

[commit-bot: I haz the power, 2017-04-30 13:36:19]

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

[magjed_webrtc, 2017-04-30 14:55:27]

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

[commit-bot: I haz the power, 2017-04-30 14:55:33]

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-04-30 15:00:02]

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

[commit-bot: I haz the power, 2017-04-30 15:00:03]

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

[magjed_webrtc, 2017-04-30 15:39:10]

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

[commit-bot: I haz the power, 2017-04-30 15:39:12]

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-04-30 15:39:23]

Patchset #3 (id:40001) has been deleted

[magjed_webrtc, 2017-04-30 15:39:30]

Patchset #2 (id:20001) has been deleted

[magjed_webrtc, 2017-04-30 15:39:33]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-04-30 15:43:41]

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

[commit-bot: I haz the power, 2017-04-30 15:43:42]

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

[magjed_webrtc, 2017-05-01 14:08:11]

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

[commit-bot: I haz the power, 2017-05-01 14:08: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-01 14:12:45]

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

[commit-bot: I haz the power, 2017-05-01 14:12:47]

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

[magjed_webrtc, 2017-05-01 14:59:59]

Patchset #1 (id:60001) has been deleted

[magjed_webrtc, 2017-05-01 15:08:24]

Description was changed from

==========
Wip VideoFrameBuffer interface change
==========

to

==========
Add support for multiple pixel formats in VideoFrameBuffer

VideoFrameBuffer is currently hard coded to be either I420 or Native.
This CL makes it more generic by moving the I420 specific functions into
it's own class, and adds an enum pixel tag that represents the pixel
format. Each pixel format is then represented as a subclass. See
webrtc/api/video/video_frame_buffer.h for more info.

This CL also adds support for representing I444 in VideoFrameBuffer.
==========

[magjed_webrtc, 2017-05-02 09:21:33]

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

[magjed_webrtc, 2017-05-02 09:21:47]

Nisse - please take a look.

[nisse-webrtc, 2017-05-02 09:52:37]

https://codereview.webrtc.org/2847383002/diff/80001/webrtc/api/video/video_fr...
File webrtc/api/video/video_frame_buffer.h (right):

https://codereview.webrtc.org/2847383002/diff/80001/webrtc/api/video/video_fr...
webrtc/api/video/video_frame_buffer.h:90: int ChromaWidth() const { return
(width() + 1) / 2; }
Implementing these separately in the two interfaces defeats the purpose of these
methods since one would need to always have separate code for I420 and I444.

I'd suggest having a YuvBufferInterface, with ChromaWidth and ChromaHeight
methods deriving the value from Format().

And have methods

  ToI420() which returns a YuvBufferInterface, promising that it really is I420.

  ToYuv() which returns an YuvBufferInterface, with identical Format(), which
can be either kI420 or kI444.

If we really want to have separate C++ interfaces for I420 and I444, those
interfaces should inherit YuvBufferInterface, and we could add an ToI444 method
too. But I don't think that's essential.

I haven't though about the transition issues, how hard do you think it is?

[magjed_webrtc, 2017-05-03 11:08:49]

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

[commit-bot: I haz the power, 2017-05-03 11:08:56]

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-03 11:13:27]

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

[commit-bot: I haz the power, 2017-05-03 11:13:28]

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

[magjed_webrtc, 2017-05-03 11:19:29]

Patchset #2 (id:100001) has been deleted

[magjed_webrtc, 2017-05-03 11:34:07]

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

[commit-bot: I haz the power, 2017-05-03 11:34:09]

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-03 11:37:18]

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

[commit-bot: I haz the power, 2017-05-03 11:37:19]

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

[magjed_webrtc, 2017-05-03 11:47:45]

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

[commit-bot: I haz the power, 2017-05-03 11:47:52]

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-03 11:52:09]

Patchset #2 (id:120001) has been deleted

[commit-bot: I haz the power, 2017-05-03 11:52:19]

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

[commit-bot: I haz the power, 2017-05-03 11:52:20]

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

[magjed_webrtc, 2017-05-03 12:58:27]

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

[commit-bot: I haz the power, 2017-05-03 12:58:40]

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-03 13:03:55]

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

[commit-bot: I haz the power, 2017-05-03 13:03:56]

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

[magjed_webrtc, 2017-05-03 13:25:00]

Patchset #2 (id:140001) has been deleted

[magjed_webrtc, 2017-05-03 13:36:55]

https://codereview.webrtc.org/2847383002/diff/80001/webrtc/api/video/video_fr...
File webrtc/api/video/video_frame_buffer.h (right):

https://codereview.webrtc.org/2847383002/diff/80001/webrtc/api/video/video_fr...
webrtc/api/video/video_frame_buffer.h:90: int ChromaWidth() const { return
(width() + 1) / 2; }
On 2017/05/02 09:52:37, nisse-webrtc wrote:
> Implementing these separately in the two interfaces defeats the purpose of
these
> methods since one would need to always have separate code for I420 and I444.
> 
> I'd suggest having a YuvBufferInterface, with ChromaWidth and ChromaHeight
> methods deriving the value from Format().
> 
> And have methods
> 
>   ToI420() which returns a YuvBufferInterface, promising that it really is
I420.
> 
>   ToYuv() which returns an YuvBufferInterface, with identical Format(), which
> can be either kI420 or kI444.
> 
> If we really want to have separate C++ interfaces for I420 and I444, those
> interfaces should inherit YuvBufferInterface, and we could add an ToI444
method
> too. But I don't think that's essential.
> 
> I haven't though about the transition issues, how hard do you think it is?

I added a PlanarYuvBuffer interface that is common for I420 and I444, and
implemented ToI420 and ChromaWidth/ChromaHeight with switch-statements instead.

I did not add a ToYuv function though. I want to keep it as straight forward as
possible and have one function per pixel format (except kNative). Also, I
renamed the functions, because ToI420 is special and should have a different
name than the other functions that don't convert. We can always add a ToYuv
helper function outside the interface that does exactly what you want.

This CL is backwards compatible and will not break any external client. After
this CL, it should be possible to update external client as well as the rest of
the WebRTC code base, and then remove the deprecated functions without issues.

[nisse-webrtc, 2017-05-04 08:15:30]

This looks pretty good!

I think the docs needs a little more work, and I don't understand how the Legacy
class fits in the picture.

https://codereview.webrtc.org/2847383002/diff/160001/webrtc/api/video/i420_bu...
File webrtc/api/video/i420_buffer.h (left):

https://codereview.webrtc.org/2847383002/diff/160001/webrtc/api/video/i420_bu...
webrtc/api/video/i420_buffer.h:56: // TODO(nisse): Deprecated, use static method
instead.
Also deleted in cl https://codereview.webrtc.org/2854873003/ which I'm
attempting to land.

https://codereview.webrtc.org/2847383002/diff/160001/webrtc/api/video/video_f...
File webrtc/api/video/video_frame_buffer.cc (right):

https://codereview.webrtc.org/2847383002/diff/160001/webrtc/api/video/video_f...
webrtc/api/video/video_frame_buffer.cc:22: // adapting them into the new
ToI420() function.
I'm afraid I don't understand what problem this class solves. It seems to just
delegate everything (but the hardcoded PixelFormat) to the wrapped buffer?

https://codereview.webrtc.org/2847383002/diff/160001/webrtc/api/video/video_f...
webrtc/api/video/video_frame_buffer.cc:55: return
const_cast<VideoFrameBuffer*>(this)->ToI420()->DataY();
This is more forgiving in the native_handle case than the current code.
Maybe use GetI420 instead? 

It might be useful with for automatic conversion from I444 to I420, but I'm not
sure this is the right place.

https://codereview.webrtc.org/2847383002/diff/160001/webrtc/api/video/video_f...
File webrtc/api/video/video_frame_buffer.h (right):

https://codereview.webrtc.org/2847383002/diff/160001/webrtc/api/video/video_f...
webrtc/api/video/video_frame_buffer.h:28: // called that will convert to I420.
This serves as a fallback for e.g. the
Maybe add some guidelines on when it makes sense to add support for additional
formats at this level? As far as I understand, it's only useful to add formats
that can be handled efficiently by some codec.

Wording and punctuation could be improved a bit, e.g.,  ", e.g.,", not "for
e.g", and the sentence with the two "that".

https://codereview.webrtc.org/2847383002/diff/160001/webrtc/api/video/video_f...
webrtc/api/video/video_frame_buffer.h:37: enum class PixelFormat {
Is "PixelFormat" the right name for this? Maybe just "Type" aka
"VideoFrameBuffer::Type" is better? The format doesn't really apply to
individual pixels, but to the higher-level organization of the buffer.

https://codereview.webrtc.org/2847383002/diff/160001/webrtc/api/video/video_f...
webrtc/api/video/video_frame_buffer.h:38: kNative,
For kNative, if casting of the void* returned by native_handle() is replaced by
casting of the VideoFrameBuffer itself, that should eliminate a couple of
NativeHandle classes and make related code simpler, I hope? 

Are the NativeHandle and the corresponding NativeHandleBuffer always created
together in the current code? I seem to remember they where separated in the
Android jni code.

https://codereview.webrtc.org/2847383002/diff/160001/webrtc/api/video/video_f...
webrtc/api/video/video_frame_buffer.h:57: // These functions should only be
called if Format() is of the correct type.
With "should", what's the failure behavior? Are they expected to crash, or just
return nullptr? I think we should decide on one or the other, and clarify the
comment accordingly.

[magjed_webrtc, 2017-05-04 11:50:44]

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

[commit-bot: I haz the power, 2017-05-04 11:50:54]

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-04 11:52:24]

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

[commit-bot: I haz the power, 2017-05-04 11:52:25]

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

[magjed_webrtc, 2017-05-04 12:12:04]

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

[commit-bot: I haz the power, 2017-05-04 12:12:11]

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-04 12:12:22]

Patchset #3 (id:180001) has been deleted

[magjed_webrtc, 2017-05-04 12:12:30]

Patchset #3 (id:200001) has been deleted

[magjed_webrtc, 2017-05-04 12:12:38]

Patchset #3 (id:220001) has been deleted

[commit-bot: I haz the power, 2017-05-04 12:17:01]

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

[commit-bot: I haz the power, 2017-05-04 12:17:03]

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

[magjed_webrtc, 2017-05-04 12:25:13]

https://codereview.webrtc.org/2847383002/diff/160001/webrtc/api/video/i420_bu...
File webrtc/api/video/i420_buffer.h (left):

https://codereview.webrtc.org/2847383002/diff/160001/webrtc/api/video/i420_bu...
webrtc/api/video/i420_buffer.h:56: // TODO(nisse): Deprecated, use static method
instead.
On 2017/05/04 08:15:29, nisse-webrtc wrote:
> Also deleted in cl https://codereview.webrtc.org/2854873003/ which I'm
> attempting to land.

Wops, this is a mistake from rebasing. I'm trying to not make unrelated changes.

https://codereview.webrtc.org/2847383002/diff/160001/webrtc/api/video/video_f...
File webrtc/api/video/video_frame_buffer.cc (right):

https://codereview.webrtc.org/2847383002/diff/160001/webrtc/api/video/video_f...
webrtc/api/video/video_frame_buffer.cc:22: // adapting them into the new
ToI420() function.
On 2017/05/04 08:15:29, nisse-webrtc wrote:
> I'm afraid I don't understand what problem this class solves. It seems to just
> delegate everything (but the hardcoded PixelFormat) to the wrapped buffer?

The problem it solves is providing a default implementation of ToI420. I want to
return a PlanarYuvBuffer, but what I have is a VideoFrameBuffer. The plan is to
make ToI420 pure virtual, but we need to update external clients first. What we
have today is NativeToI420Buffer which returns a VideoFrameBuffer (which is not
of the type PlanarYuvBuffer). This adapter allows us to land this CL without
breaking external clients. They should switch from implementing
NativeToI420Buffer to implementing ToI420 asap.

https://codereview.webrtc.org/2847383002/diff/160001/webrtc/api/video/video_f...
webrtc/api/video/video_frame_buffer.cc:55: return
const_cast<VideoFrameBuffer*>(this)->ToI420()->DataY();
On 2017/05/04 08:15:29, nisse-webrtc wrote:
> This is more forgiving in the native_handle case than the current code.
> Maybe use GetI420 instead? 
> 
> It might be useful with for automatic conversion from I444 to I420, but I'm
not
> sure this is the right place.

Done. Just to clarify, these functions will soon be removed anyway.

https://codereview.webrtc.org/2847383002/diff/160001/webrtc/api/video/video_f...
File webrtc/api/video/video_frame_buffer.h (right):

https://codereview.webrtc.org/2847383002/diff/160001/webrtc/api/video/video_f...
webrtc/api/video/video_frame_buffer.h:28: // called that will convert to I420.
This serves as a fallback for e.g. the
On 2017/05/04 08:15:29, nisse-webrtc wrote:
> Maybe add some guidelines on when it makes sense to add support for additional
> formats at this level? As far as I understand, it's only useful to add formats
> that can be handled efficiently by some codec.
> 
> Wording and punctuation could be improved a bit, e.g.,  ", e.g.,", not "for
> e.g", and the sentence with the two "that".

Done. It's a bit more tricky to describe when it's useful to add new formats.
For example, I444 is needed because our internal SW decoder can produce such
frames.

https://codereview.webrtc.org/2847383002/diff/160001/webrtc/api/video/video_f...
webrtc/api/video/video_frame_buffer.h:37: enum class PixelFormat {
On 2017/05/04 08:15:29, nisse-webrtc wrote:
> Is "PixelFormat" the right name for this? Maybe just "Type" aka
> "VideoFrameBuffer::Type" is better? The format doesn't really apply to
> individual pixels, but to the higher-level organization of the buffer.

Done, I also prefer just Type. I changed the Format() name to type(). Type()
does not work because of the naming conflict, but I think it's ok to use lower
case even if it's a virtual function (we also already do this for width/height).
The style guide says:
"Accessors and mutators (get and set functions) may be named like variables.
These often correspond to actual member variables, but this is not required. For
example, int count() and void set_count(int count)."

https://codereview.webrtc.org/2847383002/diff/160001/webrtc/api/video/video_f...
webrtc/api/video/video_frame_buffer.h:38: kNative,
On 2017/05/04 08:15:29, nisse-webrtc wrote:
> For kNative, if casting of the void* returned by native_handle() is replaced
by
> casting of the VideoFrameBuffer itself, that should eliminate a couple of
> NativeHandle classes and make related code simpler, I hope? 
> 
> Are the NativeHandle and the corresponding NativeHandleBuffer always created
> together in the current code? I seem to remember they where separated in the
> Android jni code. 

Yes, we will be able to simplify the code after this. The Android jni case won't
be a problem.

https://codereview.webrtc.org/2847383002/diff/160001/webrtc/api/video/video_f...
webrtc/api/video/video_frame_buffer.h:57: // These functions should only be
called if Format() is of the correct type.
On 2017/05/04 08:15:29, nisse-webrtc wrote:
> With "should", what's the failure behavior? Are they expected to crash, or
just
> return nullptr? I think we should decide on one or the other, and clarify the
> comment accordingly.

Right now I'm enforcing it with RTC_NOTREACHED and then returning null. This
will crash in debug builds and return null in release builds. I updated the
comment to just say undefined behavior when called with incorrect format.

[nisse-webrtc, 2017-05-05 11:33:05]

https://codereview.webrtc.org/2847383002/diff/160001/webrtc/api/video/video_f...
File webrtc/api/video/video_frame_buffer.cc (right):

https://codereview.webrtc.org/2847383002/diff/160001/webrtc/api/video/video_f...
webrtc/api/video/video_frame_buffer.cc:22: // adapting them into the new
ToI420() function.
On 2017/05/04 12:25:13, magjed_webrtc wrote:
> On 2017/05/04 08:15:29, nisse-webrtc wrote:
> > I'm afraid I don't understand what problem this class solves. It seems to
just
> > delegate everything (but the hardcoded PixelFormat) to the wrapped buffer?
> 
> The problem it solves is providing a default implementation of ToI420. I want
to
> return a PlanarYuvBuffer, but what I have is a VideoFrameBuffer. The plan is
to
> make ToI420 pure virtual, but we need to update external clients first. What
we
> have today is NativeToI420Buffer which returns a VideoFrameBuffer (which is
not
> of the type PlanarYuvBuffer). This adapter allows us to land this CL without
> breaking external clients. They should switch from implementing
> NativeToI420Buffer to implementing ToI420 asap.

I see. If we keep this, I think the comment should explain the return type
mismatch.

But maybe we can do this in some simpler way? It would be fine with me to let
the (temporary) default implementation of ToI420 call NativeToI420, DCHECK the
type, and then do an explicit cast to YuvFrameBuffer. Or even simpler, any
reason why

rtc::scoped_refptr<PlanarYuvBuffer> VideoFrameBuffer::ToI420() {
   return NativeToI420Buffer()->GetI420();
}

isn't good enough?

https://codereview.webrtc.org/2847383002/diff/160001/webrtc/api/video/video_f...
webrtc/api/video/video_frame_buffer.cc:103: rtc::scoped_refptr<I420Buffer>
i420_buffer;
Merge declaration with below assignment?

https://codereview.webrtc.org/2847383002/diff/160001/webrtc/api/video/video_f...
File webrtc/api/video/video_frame_buffer.h (right):

https://codereview.webrtc.org/2847383002/diff/160001/webrtc/api/video/video_f...
webrtc/api/video/video_frame_buffer.h:57: // These functions should only be
called if Format() is of the correct type.
On 2017/05/04 12:25:13, magjed_webrtc wrote:
> On 2017/05/04 08:15:29, nisse-webrtc wrote:
> > With "should", what's the failure behavior? Are they expected to crash, or
> just
> > return nullptr? I think we should decide on one or the other, and clarify
the
> > comment accordingly.
> 
> Right now I'm enforcing it with RTC_NOTREACHED and then returning null. This
> will crash in debug builds and return null in release builds.

And then likely crash in release builds too, since we do *not* want to clutter
up calling code with null checks and error handling.
 
> I updated the
> comment to just say undefined behavior when called with incorrect format.

Ok. Returning nullptr would also make sense to me, but we can relax this later
if we find some actual code to benefit from that.

[magjed_webrtc, 2017-05-05 12:25:24]

https://codereview.webrtc.org/2847383002/diff/160001/webrtc/api/video/video_f...
File webrtc/api/video/video_frame_buffer.cc (right):

https://codereview.webrtc.org/2847383002/diff/160001/webrtc/api/video/video_f...
webrtc/api/video/video_frame_buffer.cc:22: // adapting them into the new
ToI420() function.
On 2017/05/05 11:33:04, nisse-webrtc wrote:
> On 2017/05/04 12:25:13, magjed_webrtc wrote:
> > On 2017/05/04 08:15:29, nisse-webrtc wrote:
> > > I'm afraid I don't understand what problem this class solves. It seems to
> just
> > > delegate everything (but the hardcoded PixelFormat) to the wrapped buffer?
> > 
> > The problem it solves is providing a default implementation of ToI420. I
want
> to
> > return a PlanarYuvBuffer, but what I have is a VideoFrameBuffer. The plan is
> to
> > make ToI420 pure virtual, but we need to update external clients first. What
> we
> > have today is NativeToI420Buffer which returns a VideoFrameBuffer (which is
> not
> > of the type PlanarYuvBuffer). This adapter allows us to land this CL without
> > breaking external clients. They should switch from implementing
> > NativeToI420Buffer to implementing ToI420 asap.
> 
> I see. If we keep this, I think the comment should explain the return type
> mismatch.
> 
> But maybe we can do this in some simpler way? It would be fine with me to let
> the (temporary) default implementation of ToI420 call NativeToI420, DCHECK the
> type, and then do an explicit cast to YuvFrameBuffer. Or even simpler, any
> reason why
> 
> rtc::scoped_refptr<PlanarYuvBuffer> VideoFrameBuffer::ToI420() {
>    return NativeToI420Buffer()->GetI420();
> }
> 
> isn't good enough?

So the problem is that existing implementations do not yet implement the new
interface, i.e. they are not using PlanarYuvBuffer and they are not implementing
GetI420() or ToI420(). So the default implementation here can't rely on any of
those, i.e. we can't cast it to PlanarYuvBuffer and we can't call GetI420(). If
we cast it to PlanarYuvBuffer (even if type is kI420), it will cause corruption
since an external client might implement VideoFrameBuffer directly without
inheriting PlanarYuvBuffer. If we call GetI420() like in your second suggestion,
we will hit the default implementation of RTC_NOTREACHED(). The default
implementation I have provided here is the only one that is guaranteed to work
with existing clients. For me this class is a typical example of an adapter that
is needed when deprecating a function. I have tried to explain it as good as I
can, and I added an extra sentence about the return type mismatch that you
wanted. Also note that this is an implementation detail and not part of any API
and that it's supposed to be removed as soon as possible.

https://codereview.webrtc.org/2847383002/diff/160001/webrtc/api/video/video_f...
webrtc/api/video/video_frame_buffer.cc:103: rtc::scoped_refptr<I420Buffer>
i420_buffer;
On 2017/05/05 11:33:04, nisse-webrtc wrote:
> Merge declaration with below assignment?

Not allowed to have declarations inside switch-statements.

[nisse-webrtc, 2017-05-05 12:45:29]

https://codereview.webrtc.org/2847383002/diff/160001/webrtc/api/video/video_f...
File webrtc/api/video/video_frame_buffer.cc (right):

https://codereview.webrtc.org/2847383002/diff/160001/webrtc/api/video/video_f...
webrtc/api/video/video_frame_buffer.cc:22: // adapting them into the new
ToI420() function.
On 2017/05/05 12:25:24, magjed_webrtc wrote:
> On 2017/05/05 11:33:04, nisse-webrtc wrote:
> > On 2017/05/04 12:25:13, magjed_webrtc wrote:
> > > On 2017/05/04 08:15:29, nisse-webrtc wrote:
> > > > I'm afraid I don't understand what problem this class solves. It seems
to
> > just
> > > > delegate everything (but the hardcoded PixelFormat) to the wrapped
buffer?
> > > 
> > > The problem it solves is providing a default implementation of ToI420. I
> want
> > to
> > > return a PlanarYuvBuffer, but what I have is a VideoFrameBuffer. The plan
is
> > to
> > > make ToI420 pure virtual, but we need to update external clients first.
What
> > we
> > > have today is NativeToI420Buffer which returns a VideoFrameBuffer (which
is
> > not
> > > of the type PlanarYuvBuffer). This adapter allows us to land this CL
without
> > > breaking external clients. They should switch from implementing
> > > NativeToI420Buffer to implementing ToI420 asap.
> > 
> > I see. If we keep this, I think the comment should explain the return type
> > mismatch.
> > 
> > But maybe we can do this in some simpler way? It would be fine with me to
let
> > the (temporary) default implementation of ToI420 call NativeToI420, DCHECK
the
> > type, and then do an explicit cast to YuvFrameBuffer. Or even simpler, any
> > reason why
> > 
> > rtc::scoped_refptr<PlanarYuvBuffer> VideoFrameBuffer::ToI420() {
> >    return NativeToI420Buffer()->GetI420();
> > }
> > 
> > isn't good enough?
> 
> So the problem is that existing implementations do not yet implement the new
> interface, i.e. they are not using PlanarYuvBuffer and they are not
implementing
> GetI420() or ToI420(). So the default implementation here can't rely on any of
> those, i.e. we can't cast it to PlanarYuvBuffer and we can't call GetI420().
If
> we cast it to PlanarYuvBuffer (even if type is kI420), it will cause
corruption
> since an external client might implement VideoFrameBuffer directly without
> inheriting PlanarYuvBuffer. If we call GetI420() like in your second
suggestion,
> we will hit the default implementation of RTC_NOTREACHED(). The default
> implementation I have provided here is the only one that is guaranteed to work
> with existing clients. For me this class is a typical example of an adapter
that
> is needed when deprecating a function. I have tried to explain it as good as I
> can, and I added an extra sentence about the return type mismatch that you
> wanted. Also note that this is an implementation detail and not part of any
API
> and that it's supposed to be removed as soon as possible.

I think I finally get it. So VideoFrameBuffer has default implementations of
ToI420 and NativeToI420, calling each other (so subclasses can implement one or
the other). But it doesn't have any working default implementations of GetI420
(see below) and GetI444.

https://codereview.webrtc.org/2847383002/diff/160001/webrtc/api/video/video_f...
webrtc/api/video/video_frame_buffer.cc:93: RTC_NOTREACHED();
Wouldn't we need a working default implementation of this method, to start using
it? It could perhaps be implemented as 

  return new rtc::RefCountedObject<LegacyI420Wrapper>(this);

https://codereview.webrtc.org/2847383002/diff/160001/webrtc/api/video/video_f...
webrtc/api/video/video_frame_buffer.cc:103: rtc::scoped_refptr<I420Buffer>
i420_buffer;
On 2017/05/05 12:25:24, magjed_webrtc wrote:
> On 2017/05/05 11:33:04, nisse-webrtc wrote:
> > Merge declaration with below assignment?
> 
> Not allowed to have declarations inside switch-statements.

So you'd need to add a pair of braces to declare variables local to one case,

case ...: {
  declarations
  ...
}

I think I'd prefer locality, but it's a question of taste so
I'll leave it to your judgement.

[magjed_webrtc, 2017-05-06 19:01:45]

https://codereview.webrtc.org/2847383002/diff/160001/webrtc/api/video/video_f...
File webrtc/api/video/video_frame_buffer.cc (right):

https://codereview.webrtc.org/2847383002/diff/160001/webrtc/api/video/video_f...
webrtc/api/video/video_frame_buffer.cc:93: RTC_NOTREACHED();
On 2017/05/05 12:45:28, nisse-webrtc wrote:
> Wouldn't we need a working default implementation of this method, to start
using
> it? It could perhaps be implemented as 
> 
>   return new rtc::RefCountedObject<LegacyI420Wrapper>(this);

Yes, done. When I think about, these GetXXX functions will not need to be
virtual since the returned subclass is of a known type, so we can just
static_cast. We will have to use the adapter class for GetI420() in the
transition period, but we will be able to move to a static_cast here as well. I
upgraded the DCHECKs to CHECKs so we always crash since it's now dangerous to
continue and updated the documentation accordingly.

https://codereview.webrtc.org/2847383002/diff/160001/webrtc/api/video/video_f...
webrtc/api/video/video_frame_buffer.cc:103: rtc::scoped_refptr<I420Buffer>
i420_buffer;
On 2017/05/05 12:45:29, nisse-webrtc wrote:
> On 2017/05/05 12:25:24, magjed_webrtc wrote:
> > On 2017/05/05 11:33:04, nisse-webrtc wrote:
> > > Merge declaration with below assignment?
> > 
> > Not allowed to have declarations inside switch-statements.
> 
> So you'd need to add a pair of braces to declare variables local to one case,
> 
> case ...: {
>   declarations
>   ...
> }
> 
> I think I'd prefer locality, but it's a question of taste so
> I'll leave it to your judgement.

Thanks, I didn't know about this. Done.

[nisse-webrtc, 2017-05-08 06:43:11]

lgtm!

Plase update cl title and description now that the code no longer uses the term
"pixel format".

https://codereview.webrtc.org/2847383002/diff/160001/webrtc/api/video/video_f...
File webrtc/api/video/video_frame_buffer.cc (right):

https://codereview.webrtc.org/2847383002/diff/160001/webrtc/api/video/video_f...
webrtc/api/video/video_frame_buffer.cc:93: RTC_NOTREACHED();
On 2017/05/06 19:01:45, magjed_webrtc wrote:
> On 2017/05/05 12:45:28, nisse-webrtc wrote:
> > Wouldn't we need a working default implementation of this method, to start
> using
> > it? It could perhaps be implemented as 
> > 
> >   return new rtc::RefCountedObject<LegacyI420Wrapper>(this);
> 
> Yes, done. When I think about, these GetXXX functions will not need to be
> virtual since the returned subclass is of a known type, so we can just
> static_cast. We will have to use the adapter class for GetI420() in the
> transition period, but we will be able to move to a static_cast here as well.
I
> upgraded the DCHECKs to CHECKs so we always crash since it's now dangerous to
> continue and updated the documentation accordingly.

I still think it might make sense to have GetFoo be virtual and return null if
the object isn't of the right type. But we can do that generalization later,
if/when the need arises.

https://codereview.webrtc.org/2847383002/diff/160001/webrtc/api/video/video_f...
webrtc/api/video/video_frame_buffer.cc:103: rtc::scoped_refptr<I420Buffer>
i420_buffer;
On 2017/05/06 19:01:45, magjed_webrtc wrote:
> On 2017/05/05 12:45:29, nisse-webrtc wrote:
> > On 2017/05/05 12:25:24, magjed_webrtc wrote:
> > > On 2017/05/05 11:33:04, nisse-webrtc wrote:
> > > > Merge declaration with below assignment?
> > > 
> > > Not allowed to have declarations inside switch-statements.
> > 
> > So you'd need to add a pair of braces to declare variables local to one
case,
> > 
> > case ...: {
> >   declarations
> >   ...
> > }
> > 
> > I think I'd prefer locality, but it's a question of taste so
> > I'll leave it to your judgement.
> 
> Thanks, I didn't know about this. Done.

I guess the reason you can't have declarations in a switch statement is that the
compiler can't infer which objects need destruction at the end of the block.
You'd get code similar to this:

#include <cstdio>

class Foo {
 public:
  Foo() {
    printf("Constructed\n");
  }
  ~Foo() {
    printf("Destructed?\n");
  }
};

int main (int argc, char **argv) {
  if (argc > 1)
    goto skip;
  Foo foo;
skip:
  return 0;
}

which fails to compile like this:

$ g++ -Wall goto.cc 
goto.cc: In function ‘int main(int, char**)’:
goto.cc:17:1: error: jump to label ‘skip’ [-fpermissive]
 skip:
 ^
goto.cc:15:10: error:   from here [-fpermissive]
     goto skip;
          ^
goto.cc:16:7: error:   crosses initialization of ‘Foo foo’
   Foo foo;
       ^

Adding braces eliminates the problem by restricting the lifetime to a block
without any jump targets (case or labels) in the middle. Which in this case is
the intended meaning.

[magjed_webrtc, 2017-05-08 10:17:14]

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

[commit-bot: I haz the power, 2017-05-08 10:17:21]

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

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

[commit-bot: I haz the power, 2017-05-08 10:22:32]

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

[commit-bot: I haz the power, 2017-05-08 10:22:33]

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

[magjed_webrtc, 2017-05-08 10:26:31]

Description was changed from

==========
Add support for multiple pixel formats in VideoFrameBuffer

VideoFrameBuffer is currently hard coded to be either I420 or Native.
This CL makes it more generic by moving the I420 specific functions into
it's own class, and adds an enum pixel tag that represents the pixel
format. Each pixel format is then represented as a subclass. See
webrtc/api/video/video_frame_buffer.h for more info.

This CL also adds support for representing I444 in VideoFrameBuffer.
==========

to

==========
Add support for I444 in VideoFrameBuffer

VideoFrameBuffer is currently hard coded to be either I420 or Native.
This CL makes VideoFrameBuffer more generic by moving the I420 specific
functions into their own class, and adds an enum tag that represents the
format and storage type of the buffer. Each buffer type is then
represented as a subclass. See webrtc/api/video/video_frame_buffer.h for
more info.

This CL also adds support for representing I444 in VideoFrameBuffer
using the new interface. Possible future buffer type candidates are
RGB and NV12.
==========

[magjed_webrtc, 2017-05-08 10:27:23]

Stefan - please take a look.

[magjed_webrtc, 2017-05-11 09:51:53]

Description was changed from

==========
Add support for I444 in VideoFrameBuffer

VideoFrameBuffer is currently hard coded to be either I420 or Native.
This CL makes VideoFrameBuffer more generic by moving the I420 specific
functions into their own class, and adds an enum tag that represents the
format and storage type of the buffer. Each buffer type is then
represented as a subclass. See webrtc/api/video/video_frame_buffer.h for
more info.

This CL also adds support for representing I444 in VideoFrameBuffer
using the new interface. Possible future buffer type candidates are
RGB and NV12.
==========

to

==========
Add support for I444 in VideoFrameBuffer

VideoFrameBuffer is currently hard coded to be either I420 or Native.
This CL makes VideoFrameBuffer more generic by moving the I420 specific
functions into their own class, and adds an enum tag that represents the
format and storage type of the buffer. Each buffer type is then
represented as a subclass. See webrtc/api/video/video_frame_buffer.h for
more info.

This CL also adds support for representing I444 in VideoFrameBuffer
using the new interface. Possible future buffer type candidates are
RGB and NV12.

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

[magjed_webrtc, 2017-05-11 09:52:14]

Description was changed from

==========
Add support for I444 in VideoFrameBuffer

VideoFrameBuffer is currently hard coded to be either I420 or Native.
This CL makes VideoFrameBuffer more generic by moving the I420 specific
functions into their own class, and adds an enum tag that represents the
format and storage type of the buffer. Each buffer type is then
represented as a subclass. See webrtc/api/video/video_frame_buffer.h for
more info.

This CL also adds support for representing I444 in VideoFrameBuffer
using the new interface. Possible future buffer type candidates are
RGB and NV12.

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

to

==========
Add support for I444 in VideoFrameBuffer

VideoFrameBuffer is currently hard coded to be either I420 or Native.
This CL makes VideoFrameBuffer more generic by moving the I420 specific
functions into their own class, and adds an enum tag that represents the
format and storage type of the buffer. Each buffer type is then
represented as a subclass. See webrtc/api/video/video_frame_buffer.h for
more info.

This CL also adds support for representing I444 in VideoFrameBuffer
using the new interface. Possible future buffer type candidates are
RGB and NV12.

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

[magjed_webrtc, 2017-05-11 11:02:37]

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

[commit-bot: I haz the power, 2017-05-11 11:02:45]

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-11 11:11:25]

Description was changed from

==========
Add support for I444 in VideoFrameBuffer

VideoFrameBuffer is currently hard coded to be either I420 or Native.
This CL makes VideoFrameBuffer more generic by moving the I420 specific
functions into their own class, and adds an enum tag that represents the
format and storage type of the buffer. Each buffer type is then
represented as a subclass. See webrtc/api/video/video_frame_buffer.h for
more info.

This CL also adds support for representing I444 in VideoFrameBuffer
using the new interface. Possible future buffer type candidates are
RGB and NV12.

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

to

==========
Add support for I444 in VideoFrameBuffer

VideoFrameBuffer is currently hard coded to be either I420 or Native.
This CL makes VideoFrameBuffer more generic by moving the I420 specific
functions into their own class, and adds an enum tag that represents the
format and storage type of the buffer. Each buffer type is then
represented as a subclass. See webrtc/api/video/video_frame_buffer.h for
more info.

This CL also adds support for representing I444 in VideoFrameBuffer
using the new interface. Possible future buffer type candidates are
RGB and NV12.

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

[commit-bot: I haz the power, 2017-05-11 11:57:29]

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

[commit-bot: I haz the power, 2017-05-11 11:57:29]

Dry run: This issue passed the CQ dry run.

[magjed_webrtc, 2017-05-11 12:09:39]

The CQ bit was checked by magjed@webrtc.org

[magjed_webrtc, 2017-05-11 12:09:39]

The patchset sent to the CQ was uploaded after l-g-t-m from nisse@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2847383002/#ps320001 (title:
"Rebase")

[commit-bot: I haz the power, 2017-05-11 12:09:46]

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-11 12:11:58]

CQ is committing da patch.

Bot data: {"patchset_id": 320001, "attempt_start_ts": 1494504579274220,
"parent_rev": "28e9433b9533e5be96a58f07b432c842694c37fb", "commit_rev":
"712338eed24a8d68699b331acad12edf340ac929"}

[commit-bot: I haz the power, 2017-05-11 12:12:03]

Description was changed from

==========
Add support for I444 in VideoFrameBuffer

VideoFrameBuffer is currently hard coded to be either I420 or Native.
This CL makes VideoFrameBuffer more generic by moving the I420 specific
functions into their own class, and adds an enum tag that represents the
format and storage type of the buffer. Each buffer type is then
represented as a subclass. See webrtc/api/video/video_frame_buffer.h for
more info.

This CL also adds support for representing I444 in VideoFrameBuffer
using the new interface. Possible future buffer type candidates are
RGB and NV12.

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

to

==========
Add support for I444 in VideoFrameBuffer

VideoFrameBuffer is currently hard coded to be either I420 or Native.
This CL makes VideoFrameBuffer more generic by moving the I420 specific
functions into their own class, and adds an enum tag that represents the
format and storage type of the buffer. Each buffer type is then
represented as a subclass. See webrtc/api/video/video_frame_buffer.h for
more info.

This CL also adds support for representing I444 in VideoFrameBuffer
using the new interface. Possible future buffer type candidates are
RGB and NV12.

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

Review-Url: https://codereview.webrtc.org/2847383002
Cr-Commit-Position: refs/heads/master@{#18098}
Committed:
https://chromium.googlesource.com/external/webrtc/+/712338eed24a8d68699b331ac...
==========

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

Committed patchset #7 (id:320001) as
https://chromium.googlesource.com/external/webrtc/+/712338eed24a8d68699b331ac...

[Yuwei, 2017-05-11 21:46:59]

On 2017/05/11 12:12:05, commit-bot: I haz the power wrote:
> Committed patchset #7 (id:320001) as
>
https://chromium.googlesource.com/external/webrtc/+/712338eed24a8d68699b331ac...

Thanks for fixing this! Should I just go to hook this up in
VP9DecoderImpl::ReturnFrame or are you guys going to do this?

[nisse-webrtc, 2017-05-12 08:49:28]

On 2017/05/11 21:46:59, Yuwei wrote:
> On 2017/05/11 12:12:05, commit-bot: I haz the power wrote:
> > Committed patchset #7 (id:320001) as
> >
>
https://chromium.googlesource.com/external/webrtc/+/712338eed24a8d68699b331ac...
> 
> Thanks for fixing this! Should I just go to hook this up in
> VP9DecoderImpl::ReturnFrame or are you guys going to do this?

Not sure how to do this with minimal breakage. I'm afraid the safest way
is still to have VP9DecoderImpl convert to I420 by default, with some option to
enable
generation of I444 frames.

Magnus, what's your plan? How much code needs to be updated with calls to
ToI420?

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

On 2017/05/12 08:49:28, nisse-webrtc wrote:
> On 2017/05/11 21:46:59, Yuwei wrote:
> > On 2017/05/11 12:12:05, commit-bot: I haz the power wrote:
> > > Committed patchset #7 (id:320001) as
> > >
> >
>
https://chromium.googlesource.com/external/webrtc/+/712338eed24a8d68699b331ac...
> > 
> > Thanks for fixing this! Should I just go to hook this up in
> > VP9DecoderImpl::ReturnFrame or are you guys going to do this?
> 
> Not sure how to do this with minimal breakage. I'm afraid the safest way
> is still to have VP9DecoderImpl convert to I420 by default, with some option
to
> enable
> generation of I444 frames.
> 
> Magnus, what's your plan? How much code needs to be updated with calls to
> ToI420?

Sorry for late reply, I had missed this.

Yuwei - please go ahead and make a CL for hooking this up in
VP9DecoderImpl::ReturnFrame, but I need to clean up some places in WebRTC before
you can land it.

[guidou, 2017-06-08 08:28:30]

A revert of this CL (patchset #7 id:320001) has been created in
https://codereview.webrtc.org/2924263002/ by guidou@webrtc.org.

The reason for reverting is: Suspect of breaking FYI bots.
See
https://build.chromium.org/p/chromium.webrtc.fyi/builders/Win8%20Tester/build...

[4100:4488:0607/163040.910:9150875:FATAL:webrtc_video_frame_adapter.cc(98)]
Check failed: false. 
Backtrace:
	base::debug::StackTrace::StackTrace [0x01CD71F7+55]
	base::debug::StackTrace::StackTrace [0x01CF179A+10]
	content::WebRtcVideoFrameAdapter::NativeToI420Buffer [0x03229C01+305]
	webrtc::VideoFrameBuffer::ToI420 [0x012640E7+39]
	webrtc::VP8EncoderImpl::Encode [0x036DAA93+179]
	webrtc::VCMGenericEncoder::Encode [0x036D04B4+333]
	webrtc::vcm::VideoSender::AddVideoFrame [0x036CE35B+796]
	webrtc::ViEEncoder::EncodeVideoFrame [0x036AF2E6+884]
	webrtc::ViEEncoder::EncodeTask::Run [0x036B04C7+215]
	rtc::TaskQueue::PostTask [0x02847B8B+194]
	base::internal::Invoker<base::internal::BindState<void
(__cdecl*)(std::unique_ptr<webcrypto::`anonymous
namespace'::DeriveBitsState,std::default_delete<webcrypto::`anonymous
namespace'::DeriveBitsState>
>),base::internal::PassedWrapper<std::unique_ptr<web [0x01143F3E+31]
	base::internal::Invoker<base::internal::BindState<void
(__cdecl*)(std::unique_ptr<webcrypto::`anonymous
namespace'::DeriveBitsState,std::default_delete<webcrypto::`anonymous
namespace'::DeriveBitsState>
>),base::internal::PassedWrapper<std::unique_ptr<web [0x01148086+22]
	base::debug::TaskAnnotator::RunTask [0x01CFD279+409]
	base::MessageLoop::RunTask [0x01C8FFF1+1233]
	base::MessageLoop::DoWork [0x01C8F3AD+765]
	base::MessagePumpDefault::Run [0x01CFE12B+219]
	base::MessageLoop::Run [0x01C8FB0B+107]
	base::RunLoop::Run [0x01C84D53+147]
	base::Thread::Run [0x01CC5C5D+173]
	base::Thread::ThreadMain [0x01CC676E+622]
	base::PlatformThread::Sleep [0x01C8B762+290]
	BaseThreadInitThunk [0x75E17C04+36]
	RtlInitializeExceptionChain [0x7798AB8F+143]
	RtlInitializeExceptionChain [0x7798AB5A+90].

[guidou, 2017-06-08 08:31:14]

On 2017/06/08 08:28:30, guidou wrote:
> A revert of this CL (patchset #7 id:320001) has been created in
> https://codereview.webrtc.org/2924263002/ by mailto:guidou@webrtc.org.
> 
> The reason for reverting is: Suspect of breaking FYI bots.
> See
>
https://build.chromium.org/p/chromium.webrtc.fyi/builders/Win8%20Tester/build...
> 
> [4100:4488:0607/163040.910:9150875:FATAL:webrtc_video_frame_adapter.cc(98)]
> Check failed: false. 
> Backtrace:
> 	base::debug::StackTrace::StackTrace [0x01CD71F7+55]
> 	base::debug::StackTrace::StackTrace [0x01CF179A+10]
> 	content::WebRtcVideoFrameAdapter::NativeToI420Buffer [0x03229C01+305]
> 	webrtc::VideoFrameBuffer::ToI420 [0x012640E7+39]
> 	webrtc::VP8EncoderImpl::Encode [0x036DAA93+179]
> 	webrtc::VCMGenericEncoder::Encode [0x036D04B4+333]
> 	webrtc::vcm::VideoSender::AddVideoFrame [0x036CE35B+796]
> 	webrtc::ViEEncoder::EncodeVideoFrame [0x036AF2E6+884]
> 	webrtc::ViEEncoder::EncodeTask::Run [0x036B04C7+215]
> 	rtc::TaskQueue::PostTask [0x02847B8B+194]
> 	base::internal::Invoker<base::internal::BindState<void
> (__cdecl*)(std::unique_ptr<webcrypto::`anonymous
> namespace'::DeriveBitsState,std::default_delete<webcrypto::`anonymous
> namespace'::DeriveBitsState>
> >),base::internal::PassedWrapper<std::unique_ptr<web [0x01143F3E+31]
> 	base::internal::Invoker<base::internal::BindState<void
> (__cdecl*)(std::unique_ptr<webcrypto::`anonymous
> namespace'::DeriveBitsState,std::default_delete<webcrypto::`anonymous
> namespace'::DeriveBitsState>
> >),base::internal::PassedWrapper<std::unique_ptr<web [0x01148086+22]
> 	base::debug::TaskAnnotator::RunTask [0x01CFD279+409]
> 	base::MessageLoop::RunTask [0x01C8FFF1+1233]
> 	base::MessageLoop::DoWork [0x01C8F3AD+765]
> 	base::MessagePumpDefault::Run [0x01CFE12B+219]
> 	base::MessageLoop::Run [0x01C8FB0B+107]
> 	base::RunLoop::Run [0x01C84D53+147]
> 	base::Thread::Run [0x01CC5C5D+173]
> 	base::Thread::ThreadMain [0x01CC676E+622]
> 	base::PlatformThread::Sleep [0x01C8B762+290]
> 	BaseThreadInitThunk [0x75E17C04+36]
> 	RtlInitializeExceptionChain [0x7798AB8F+143]
> 	RtlInitializeExceptionChain [0x7798AB5A+90].

Sorry, this was the wrong CL to revert. It's the follow-up that's suspect.

[Yuwei, 2017-06-08 20:37:48]

On 2017/05/25 14:23:30, magjed_webrtc wrote:
> On 2017/05/12 08:49:28, nisse-webrtc wrote:
> > On 2017/05/11 21:46:59, Yuwei wrote:
> > > On 2017/05/11 12:12:05, commit-bot: I haz the power wrote:
> > > > Committed patchset #7 (id:320001) as
> > > >
> > >
> >
>
https://chromium.googlesource.com/external/webrtc/+/712338eed24a8d68699b331ac...
> > > 
> > > Thanks for fixing this! Should I just go to hook this up in
> > > VP9DecoderImpl::ReturnFrame or are you guys going to do this?
> > 
> > Not sure how to do this with minimal breakage. I'm afraid the safest way
> > is still to have VP9DecoderImpl convert to I420 by default, with some option
> to
> > enable
> > generation of I444 frames.
> > 
> > Magnus, what's your plan? How much code needs to be updated with calls to
> > ToI420?
> 
> Sorry for late reply, I had missed this.
> 
> Yuwei - please go ahead and make a CL for hooking this up in
> VP9DecoderImpl::ReturnFrame, but I need to clean up some places in WebRTC
before
> you can land it.

Looks like I also missed the comment :)
I've put uploaded a CL for VP9Impl here:
https://codereview.chromium.org/2927943003/

I probably need to wait for CL 2876363003 to land it though.