Move key frame flag from StreamParserBuffer to DecoderBuffer

Move key frame flag from StreamParserBuffer to DecoderBuffer

This change is the first in a sequence of changes to enable eventual
removal of the "BrowserSeek" behavior in Chrome for Android by letting
reads from demuxer indicate if the buffer is a key frame or not.

This change
* moves |is_keyframe_| from StreamParserBuffer to DecoderBuffer |is_key_frame_|
* moves StreamParserBuffer::IsKeyframe() to DecoderBuffer::is_key_frame()
* adds DecoderBuffer::set_is_key_frame() to allow updating after buffer
  construction
* populates this field in DemuxerStream subclasses and related code
* updates unit tests, especially ChunkDemuxerTest and FFmpegDemuxerTest,
  to check keyframe expectations in stream read results for most of these tests
* updates media/mojo type DecoderBuffer type converter and unit tests to
  round-trip the key frame flag.

Later change(s) will do the actual removal of Chrome for Android
BrowserSeek logic and tests.

BUG=304234
TEST=Updated unit tests pass, including mojo_media_lib_unittests

Committed: https://crrev.com/02357c0df3949ee2ec1a9fabf22a8ee6e70cb41f
Cr-Commit-Position: refs/heads/master@{#303764}

[wolenetz, 2014-11-08 02:15:16]

wolenetz@chromium.org changed reviewers:
+ dmichael@chromium.org
- dalecurtis@chromium.org

[wolenetz, 2014-11-08 02:21:36]

Please take a look at patch set 1:
dmichael@: content/* OWNERS review
xhwang@: everything

I decided to require |is_keyframe| in DecoderBuffer::CopyFrom(...) and one of
its two constructors to help keep keyframe info an explicit part of the API so
consumers of Reads from a DemuxerStream have more reliable keyframe info. This
made the change a bit more extensive, but I believe it is worth it to offset
future bugs. WDYT?

Not all of the ChunkDemuxerTests include keyframe on demuxerstream read
expectation verification, though all of the CheckExpectedBuffers(..) uses do
this. I'm less convinced of the value-add for this verification for the
remainder of the ChunkDemuxerTests.

Finally, I have not updated mojo (TypeConverter, etc) to use the updated
DecoderBuffer. Would such need to be part of this CL, too?

Thanks!

[wolenetz, 2014-11-08 02:34:38]

On 2014/11/08 02:21:36, wolenetz wrote:
> Please take a look at patch set 1:
> dmichael@: content/* OWNERS review
> xhwang@: everything
> 
> I decided to require |is_keyframe| in DecoderBuffer::CopyFrom(...) and one of
> its two constructors to help keep keyframe info an explicit part of the API so
> consumers of Reads from a DemuxerStream have more reliable keyframe info. This
> made the change a bit more extensive, but I believe it is worth it to offset
> future bugs. WDYT?
> 
> Not all of the ChunkDemuxerTests include keyframe on demuxerstream read
> expectation verification, though all of the CheckExpectedBuffers(..) uses do
> this. I'm less convinced of the value-add for this verification for the
> remainder of the ChunkDemuxerTests.
> 
> Finally, I have not updated mojo (TypeConverter, etc) to use the updated
> DecoderBuffer. Would such need to be part of this CL, too?
> 
> Thanks!

Yeah, try bot compiler failures show I'll need to update mojo stuff as part of
this CL. For now, please take a look at the rest of patch set 1.

[dmichael (off chromium), 2014-11-10 17:22:24]

one nit, otherwise content/renderer/pepper lgtm

https://codereview.chromium.org/712593003/diff/1/content/renderer/pepper/cont...
File content/renderer/pepper/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/712593003/diff/1/content/renderer/pepper/cont...
content/renderer/pepper/content_decryptor_delegate.cc:970: false));
I usually prefer enums to bools for the sake of readability. I'll leave it up to
your other reviewer as to whether that's worth doing, since the API is in media.

Here, if you do still use a bool, please add an in-line comment, like:
false));  // is_key_frame

[xhwang, 2014-11-10 20:14:13]

Mostly looking pretty good. I only have some style/naming nits.

https://codereview.chromium.org/712593003/diff/1/media/base/decoder_buffer.h
File media/base/decoder_buffer.h (right):

https://codereview.chromium.org/712593003/diff/1/media/base/decoder_buffer.h#...
media/base/decoder_buffer.h:52: bool is_keyframe);
We don't set a lot of variables in CopyFrom, e.g. the timestamp, duration etc.
Why |is_keyframe| is special? How about we treat it the same ways as timestamp?
(set with setter after construction)

This is also consistent with the ctor on l.46.

https://codereview.chromium.org/712593003/diff/1/media/base/decoder_buffer.h#...
media/base/decoder_buffer.h:61: bool is_keyframe);
ditto.

https://codereview.chromium.org/712593003/diff/1/media/base/decoder_buffer.h#...
media/base/decoder_buffer.h:183: bool is_keyframe);
ditto

https://codereview.chromium.org/712593003/diff/1/media/base/decoder_buffer.h#...
media/base/decoder_buffer.h:197: bool is_keyframe_;
It seems to me key and frame should be two words, e.g.

http://en.wikipedia.org/wiki/Key_frame

For comparison, timestamp is one word according to wikipedia:

http://en.wikipedia.org/wiki/Timestamp

https://codereview.chromium.org/712593003/diff/1/media/base/decryptor.h
File media/base/decryptor.h (right):

https://codereview.chromium.org/712593003/diff/1/media/base/decryptor.h#newco...
media/base/decryptor.h:73: //   is responsible for setting this flag true as
appropriate.
How about: Only |data|, |data_size| and |timestamp| are set in the returned
DecoderBuffer. The callback handler is responsible for setting other fields as
appropriate.

https://codereview.chromium.org/712593003/diff/1/media/filters/ffmpeg_demuxer...
File media/filters/ffmpeg_demuxer_unittest.cc (right):

https://codereview.chromium.org/712593003/diff/1/media/filters/ffmpeg_demuxer...
media/filters/ffmpeg_demuxer_unittest.cc:111: struct ReadExpectation {
This is not POD. Add a constructor? That'll also make the creation below
cleaner.

https://codereview.chromium.org/712593003/diff/1/media/filters/ffmpeg_demuxer...
media/filters/ffmpeg_demuxer_unittest.cc:122: ReadExpectation read_expectation,
nit: pass by const-ref?

[wolenetz, 2014-11-11 22:39:41]

Please take a look at patch set 2:
xhwang@: everything,
dmichael@, FYI, there are no longer any content/ files in this CL for your
review.

Thanks!
Matt

https://codereview.chromium.org/712593003/diff/1/content/renderer/pepper/cont...
File content/renderer/pepper/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/712593003/diff/1/content/renderer/pepper/cont...
content/renderer/pepper/content_decryptor_delegate.cc:970: false));
On 2014/11/10 17:22:24, dmichael wrote:
> I usually prefer enums to bools for the sake of readability. I'll leave it up
to
> your other reviewer as to whether that's worth doing, since the API is in
media.
> 
> Here, if you do still use a bool, please add an in-line comment, like:
> false));  // is_key_frame

This file will remain unchanged in this CL by following xhwang@'s comments.
Thanks for your review of PS1, dmichael@.

https://codereview.chromium.org/712593003/diff/1/media/base/decoder_buffer.h
File media/base/decoder_buffer.h (right):

https://codereview.chromium.org/712593003/diff/1/media/base/decoder_buffer.h#...
media/base/decoder_buffer.h:52: bool is_keyframe);
On 2014/11/10 20:14:12, xhwang wrote:
> We don't set a lot of variables in CopyFrom, e.g. the timestamp, duration etc.
> Why |is_keyframe| is special? How about we treat it the same ways as
timestamp?
> (set with setter after construction)
> 
> This is also consistent with the ctor on l.46.

Done. This makes the change less extensive, too. Keeping the keyframe flag
populated correctly in code maintenance may be harder, though, as I mentioned
earlier.

https://codereview.chromium.org/712593003/diff/1/media/base/decoder_buffer.h#...
media/base/decoder_buffer.h:61: bool is_keyframe);
On 2014/11/10 20:14:12, xhwang wrote:
> ditto.

Done.

https://codereview.chromium.org/712593003/diff/1/media/base/decoder_buffer.h#...
media/base/decoder_buffer.h:183: bool is_keyframe);
On 2014/11/10 20:14:12, xhwang wrote:
> ditto

Done.

https://codereview.chromium.org/712593003/diff/1/media/base/decoder_buffer.h#...
media/base/decoder_buffer.h:197: bool is_keyframe_;
On 2014/11/10 20:14:12, xhwang wrote:
> It seems to me key and frame should be two words, e.g.
> 
> http://en.wikipedia.org/wiki/Key_frame
> 
> For comparison, timestamp is one word according to wikipedia:
> 
> http://en.wikipedia.org/wiki/Timestamp

There is much inconsistency on the web about this one. I personally agree with
your comment, though "keyframe" pre-exists all over media/ (especially in stream
parsers and in source_buffer_stream*), and it shows up even in specs and docs
like:
http://matroska.org/technical/specs/index.html
http://www.webmproject.org/docs/container/

I'll not further propagate "keyframe" in this CL, though there are over 300
instances in media/ that this CL does not change.

https://codereview.chromium.org/712593003/diff/1/media/base/decryptor.h
File media/base/decryptor.h (right):

https://codereview.chromium.org/712593003/diff/1/media/base/decryptor.h#newco...
media/base/decryptor.h:73: //   is responsible for setting this flag true as
appropriate.
On 2014/11/10 20:14:12, xhwang wrote:
> How about: Only |data|, |data_size| and |timestamp| are set in the returned
> DecoderBuffer. The callback handler is responsible for setting other fields as
> appropriate.

Done.

https://codereview.chromium.org/712593003/diff/1/media/filters/ffmpeg_demuxer...
File media/filters/ffmpeg_demuxer_unittest.cc (right):

https://codereview.chromium.org/712593003/diff/1/media/filters/ffmpeg_demuxer...
media/filters/ffmpeg_demuxer_unittest.cc:111: struct ReadExpectation {
On 2014/11/10 20:14:12, xhwang wrote:
> This is not POD. Add a constructor? That'll also make the creation below
> cleaner.

Done.

https://codereview.chromium.org/712593003/diff/1/media/filters/ffmpeg_demuxer...
media/filters/ffmpeg_demuxer_unittest.cc:122: ReadExpectation read_expectation,
On 2014/11/10 20:14:12, xhwang wrote:
> nit: pass by const-ref?

Done.

[xhwang, 2014-11-11 23:24:27]

LGTM with tiny nits. Thanks!

https://codereview.chromium.org/712593003/diff/20001/media/base/decoder_buffer.h
File media/base/decoder_buffer.h (right):

https://codereview.chromium.org/712593003/diff/20001/media/base/decoder_buffe...
media/base/decoder_buffer.h:45: // as necessary, and is_key_frame() will default
to false.
s/is_key_frame()/|is_key_frame_|

https://codereview.chromium.org/712593003/diff/20001/media/base/decoder_buffe...
media/base/decoder_buffer.h:50: // The buffer's is_key_frame() will default to
false.
ditto

https://codereview.chromium.org/712593003/diff/20001/media/base/decoder_buffe...
media/base/decoder_buffer.h:56: // is_key_frame() will default to false.
ditto

https://codereview.chromium.org/712593003/diff/20001/media/base/decoder_buffe...
media/base/decoder_buffer.h:177: // set to NULL and |buffer_size_| to 0. 
is_key_frame() will default to false.
ditto

[wolenetz, 2014-11-11 23:52:54]

Thanks for reviews.

https://codereview.chromium.org/712593003/diff/20001/media/base/decoder_buffer.h
File media/base/decoder_buffer.h (right):

https://codereview.chromium.org/712593003/diff/20001/media/base/decoder_buffe...
media/base/decoder_buffer.h:45: // as necessary, and is_key_frame() will default
to false.
On 2014/11/11 23:24:26, xhwang wrote:
> s/is_key_frame()/|is_key_frame_|

Done.

https://codereview.chromium.org/712593003/diff/20001/media/base/decoder_buffe...
media/base/decoder_buffer.h:50: // The buffer's is_key_frame() will default to
false.
On 2014/11/11 23:24:26, xhwang wrote:
> ditto

Done.

https://codereview.chromium.org/712593003/diff/20001/media/base/decoder_buffe...
media/base/decoder_buffer.h:56: // is_key_frame() will default to false.
On 2014/11/11 23:24:26, xhwang wrote:
> ditto

Done.

https://codereview.chromium.org/712593003/diff/20001/media/base/decoder_buffe...
media/base/decoder_buffer.h:177: // set to NULL and |buffer_size_| to 0. 
is_key_frame() will default to false.
On 2014/11/11 23:24:26, xhwang wrote:
> ditto

Done.

[wolenetz, 2014-11-11 23:53:01]

The CQ bit was checked by wolenetz@chromium.org

[commit-bot: I haz the power, 2014-11-11 23:54:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/712593003/40001

[commit-bot: I haz the power, 2014-11-12 00:55:53]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-11-12 00:56:59]

Patchset 3 (id:??) landed as
https://crrev.com/02357c0df3949ee2ec1a9fabf22a8ee6e70cb41f
Cr-Commit-Position: refs/heads/master@{#303764}