H264: Improve FFmpeg decoder performance by using I420BufferPool.

H264: Improve FFmpeg decoder performance by using I420BufferPool.

Had to update I420BufferPool to allow zero-initializing buffers.

BUG=chromium:500605, chromium:468365, webrtc:5428
Committed: https://crrev.com/900f97534b8ce5de579f80f332cf674d78ac1679
Cr-Commit-Position: refs/heads/master@{#11505}

[hbos, 2016-01-27 16:36:29]

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

[hbos, 2016-01-27 16:36:30]

Please take a look, Stefan

[hbos, 2016-01-28 10:46:56]

Description was changed from

==========
H264: Improve FFmpeg decoder performance by using I420BufferPool.

Had to update I420BufferPool to allow zero-initializing buffers.

BUG=chromium:500605, chromium:468365
BUG=https://bugs.chromium.org/p/webrtc/issues/detail?id=5428
==========

to

==========
H264: Improve FFmpeg decoder performance by using I420BufferPool.

Had to update I420BufferPool to allow zero-initializing buffers.

BUG=chromium:500605, chromium:468365, webrtc:5428
==========

[hbos, 2016-01-29 10:16:57]

Bump :)

https://codereview.webrtc.org/1645543003/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc (right):

https://codereview.webrtc.org/1645543003/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:92:
reinterpret_cast<H264DecoderImpl*>(context->opaque);
To reinterpret_cast or static_cast, that is the question...

[stefan-webrtc, 2016-02-01 14:02:09]

https://codereview.webrtc.org/1645543003/diff/40001/webrtc/common_video/i420_...
File webrtc/common_video/i420_buffer_pool.cc (right):

https://codereview.webrtc.org/1645543003/diff/40001/webrtc/common_video/i420_...
webrtc/common_video/i420_buffer_pool.cc:90: memset(buffer->MutableData(kYPlane),
0, buffer->DataSize());
Is it safe to assume continuous memory here?

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

https://codereview.webrtc.org/1645543003/diff/40001/webrtc/common_video/inclu...
webrtc/common_video/include/i420_buffer_pool.h:42: // buffers are not zero'd
before reuse. (This is required of buffers used by
Maybe mention why rezeroing isn't needed?

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

https://codereview.webrtc.org/1645543003/diff/40001/webrtc/common_video/video...
webrtc/common_video/video_frame_buffer.cc:85: return stride_y_ * height_ +
(stride_u_ + stride_v_) * ((height_ + 1) / 2);
Shouldn't this be the fully allocated size if you are going to use it for
memsetting?

https://codereview.webrtc.org/1645543003/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc (left):

https://codereview.webrtc.org/1645543003/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:142:
memset(video_frame->buffer(kYPlane), 0, total_size);
The memset you did in the pool doesn't memset the total_size, but only the
DataSize(), which appears to be the part of the buffer that is actually being
used, and not what was allocated?

https://codereview.webrtc.org/1645543003/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc (right):

https://codereview.webrtc.org/1645543003/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:86: // Could be
owned by decoder, |static_cast<H264DecoderImpl*>(context->opaque)|.
Should this comment be removed now?

https://codereview.webrtc.org/1645543003/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:164: delete
video_frame;
Seems odd that we delete frames when we are using a pool?

https://codereview.webrtc.org/1645543003/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.h (right):

https://codereview.webrtc.org/1645543003/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.h:56: static int
AVGetBuffer2(
Add a comment describing what these implement.

[hbos, 2016-02-02 15:46:17]

Patchset #4 (id:60001) has been deleted

[hbos, 2016-02-02 16:08:33]

Patchset #4 (id:80001) has been deleted

[hbos, 2016-02-02 16:13:03]

PTAL stefan

https://codereview.webrtc.org/1645543003/diff/40001/webrtc/common_video/i420_...
File webrtc/common_video/i420_buffer_pool.cc (right):

https://codereview.webrtc.org/1645543003/diff/40001/webrtc/common_video/i420_...
webrtc/common_video/i420_buffer_pool.cc:90: memset(buffer->MutableData(kYPlane),
0, buffer->DataSize());
On 2016/02/01 14:02:08, stefan-webrtc (holmer) wrote:
> Is it safe to assume continuous memory here?

Yes because it is a pool specifically for I420Buffers which are continuous.
Didn't DCHECK because I420Buffer does not contain |aligned_size| like VideoFrame
does and didn't think it was worth adding.

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

https://codereview.webrtc.org/1645543003/diff/40001/webrtc/common_video/inclu...
webrtc/common_video/include/i420_buffer_pool.h:42: // buffers are not zero'd
before reuse. (This is required of buffers used by
On 2016/02/01 14:02:08, stefan-webrtc (holmer) wrote:
> Maybe mention why rezeroing isn't needed?

Done.

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

https://codereview.webrtc.org/1645543003/diff/40001/webrtc/common_video/video...
webrtc/common_video/video_frame_buffer.cc:85: return stride_y_ * height_ +
(stride_u_ + stride_v_) * ((height_ + 1) / 2);
On 2016/02/01 14:02:09, stefan-webrtc (holmer) wrote:
> Shouldn't this be the fully allocated size if you are going to use it for
> memsetting?

This is the size used fir AlignedMalloc and for memset.

Apparently only the data size has to be memset, not the additional bytes due to
alignment:
https://codereview.chromium.org/383893002/diff/1/media/base/video_frame.cc.

This equation is used in two places in this file so I made a helper function.
Also made it return int because the equations was always evaluated as int.

https://codereview.webrtc.org/1645543003/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc (left):

https://codereview.webrtc.org/1645543003/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:142:
memset(video_frame->buffer(kYPlane), 0, total_size);
On 2016/02/01 14:02:09, stefan-webrtc (holmer) wrote:
> The memset you did in the pool doesn't memset the total_size, but only the
> DataSize(), which appears to be the part of the buffer that is actually being
> used, and not what was allocated?

That memset and this memset are the same. See previous comment, the extra bytes
due to memory alignment doesn't look like they need to be memset.

https://codereview.webrtc.org/1645543003/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc (right):

https://codereview.webrtc.org/1645543003/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:86: // Could be
owned by decoder, |static_cast<H264DecoderImpl*>(context->opaque)|.
On 2016/02/01 14:02:09, stefan-webrtc (holmer) wrote:
> Should this comment be removed now?

Done.

https://codereview.webrtc.org/1645543003/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:92:
reinterpret_cast<H264DecoderImpl*>(context->opaque);
On 2016/01/29 10:16:56, hbos wrote:
> To reinterpret_cast or static_cast, that is the question...

static_cast, that is the answer.

https://codereview.webrtc.org/1645543003/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.cc:164: delete
video_frame;
On 2016/02/01 14:02:09, stefan-webrtc (holmer) wrote:
> Seems odd that we delete frames when we are using a pool?

Added a comment explaining why.

https://codereview.webrtc.org/1645543003/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.h (right):

https://codereview.webrtc.org/1645543003/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/h264/h264_decoder_impl.h:56: static int
AVGetBuffer2(
On 2016/02/01 14:02:09, stefan-webrtc (holmer) wrote:
> Add a comment describing what these implement.

Done.

[hbos, 2016-02-05 10:04:36]

Reminder: PTAL stefan. In case you forgot, no stress.

[stefan-webrtc, 2016-02-05 10:18:53]

Thanks for the reminder

https://codereview.webrtc.org/1645543003/diff/100001/webrtc/common_video/incl...
File webrtc/common_video/include/video_frame_buffer.h (right):

https://codereview.webrtc.org/1645543003/diff/100001/webrtc/common_video/incl...
webrtc/common_video/include/video_frame_buffer.h:77: int DataSize() const;
I'm not sure I like this. Should we instead provide an InitializeData() method
which does the memset? We're now exposing information about how I420Buffer
stores its the buffer internally, which will be bad if we ever decide to change
it to three separate buffers.

[hbos, 2016-02-05 11:23:44]

Thanks. PTAL stefan

https://codereview.webrtc.org/1645543003/diff/100001/webrtc/common_video/incl...
File webrtc/common_video/include/video_frame_buffer.h (right):

https://codereview.webrtc.org/1645543003/diff/100001/webrtc/common_video/incl...
webrtc/common_video/include/video_frame_buffer.h:77: int DataSize() const;
On 2016/02/05 10:18:53, stefan-webrtc (holmer) wrote:
> I'm not sure I like this. Should we instead provide an InitializeData() method
> which does the memset? We're now exposing information about how I420Buffer
> stores its the buffer internally, which will be bad if we ever decide to
change
> it to three separate buffers.

Fair enough. If that changes, we will no longer be able to rely on this buffer
implementation for ffmpeg (or change so that ffmpeg does not rely on the buffer
being continuous), but at least that's DCHECKed.

[stefan-webrtc, 2016-02-05 12:45:33]

lgtm

[hbos, 2016-02-05 12:48:42]

The CQ bit was checked by hbos@webrtc.org

[commit-bot: I haz the power, 2016-02-05 12:48:44]

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

[commit-bot: I haz the power, 2016-02-05 12:50:04]

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

[commit-bot: I haz the power, 2016-02-05 12:50:05]

Try jobs failed on following builders:
  ios64_sim_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/4960)
  ios_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/12458)
  linux_gn_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_gn_rel/builds/8434)
  mac_gn_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_rel/builds/6862)

[hbos, 2016-02-05 13:04:53]

The CQ bit was checked by hbos@webrtc.org

[hbos, 2016-02-05 13:04:53]

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1645543003/#ps160001 (title:
"Make compile (data_.get()), GN deps same as GYP")

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

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

[commit-bot: I haz the power, 2016-02-05 13:07:12]

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

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

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

[hbos, 2016-02-05 13:52:47]

Error due to circular dependency common_video -> webrtc_h264 -> common_video. I
see no reason why common_video should depend on webrtc_h264. I think this was a
mistake I made earlier. Just webrtc_h264 -> common_video should be fine. CQ'ing
that.

[hbos, 2016-02-05 13:55:49]

The CQ bit was checked by hbos@webrtc.org

[hbos, 2016-02-05 13:55:49]

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1645543003/#ps180001 (title:
"common_video not unnecessarily depending on webrtc_h264, fixed circular
dependency")

[commit-bot: I haz the power, 2016-02-05 13:55:56]

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

[commit-bot: I haz the power, 2016-02-05 16:08:36]

Description was changed from

==========
H264: Improve FFmpeg decoder performance by using I420BufferPool.

Had to update I420BufferPool to allow zero-initializing buffers.

BUG=chromium:500605, chromium:468365, webrtc:5428
==========

to

==========
H264: Improve FFmpeg decoder performance by using I420BufferPool.

Had to update I420BufferPool to allow zero-initializing buffers.

BUG=chromium:500605, chromium:468365, webrtc:5428
==========

[commit-bot: I haz the power, 2016-02-05 16:08:37]

Committed patchset #8 (id:180001)

[commit-bot: I haz the power, 2016-02-05 16:08:44]

Description was changed from

==========
H264: Improve FFmpeg decoder performance by using I420BufferPool.

Had to update I420BufferPool to allow zero-initializing buffers.

BUG=chromium:500605, chromium:468365, webrtc:5428
==========

to

==========
H264: Improve FFmpeg decoder performance by using I420BufferPool.

Had to update I420BufferPool to allow zero-initializing buffers.

BUG=chromium:500605, chromium:468365, webrtc:5428

Committed: https://crrev.com/900f97534b8ce5de579f80f332cf674d78ac1679
Cr-Commit-Position: refs/heads/master@{#11505}
==========

[commit-bot: I haz the power, 2016-02-05 16:08:45]

Patchset 8 (id:??) landed as
https://crrev.com/900f97534b8ce5de579f80f332cf674d78ac1679
Cr-Commit-Position: refs/heads/master@{#11505}