SourceState: Coalesce OnNewBuffers() CB to include OnTextBuffers() behavior

SourceState: Coalesce OnNewBuffers() CB to include OnTextBuffers() behavior

Merges the new audio/video buffer callback (SourceState::OnNewBuffers)
with the new text buffer callback (SourceState::OnTextBuffers), so that
stream parsers use only OnNewBuffers for all buffer types. The only
known behavior change is to DCHECK if there are no audio, video,
or text buffers. This is a prerequisite CL for implementing MSE
coded frame processing algorithm compliance and handling SourceBuffer
append modes correctly.

R=xhwang@chromium.org
BUG=249422
TEST=All media_unittests pass on local linux with proprietary codecs enabled.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=246762

[wolenetz, 2014-01-22 22:59:21]

PTAL. This is the lowest hanging refactoring fruit along the path to getting
MSE-compliant coded frame processing implemented.

Thanks!

[acolwell GONE FROM CHROMIUM, 2014-01-23 01:32:48]

https://codereview.chromium.org/143973009/diff/1/media/filters/chunk_demuxer.cc
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/143973009/diff/1/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:186: // TODO(wolenetz): Enable "sequence" mode
logic including MSE-compliant coded
nit: I don't think you need to add this TODO since you are actively working on
this right now.

https://codereview.chromium.org/143973009/diff/1/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:778: DCHECK(!audio_buffers.empty() ||
!video_buffers.empty() || text_map);
nit: s/text_map/(text_map && !text_map->empty())/ and remove the TODO.

https://codereview.chromium.org/143973009/diff/1/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:835: for
(StreamParser::TextBufferQueueMap::const_iterator itr =
nit: Add an bool all_empty = true; before the loop and DCHECK() it before the
return to verify that we got at least 1 buffer.

https://codereview.chromium.org/143973009/diff/1/media/mp2t/mp2t_stream_parse...
File media/mp2t/mp2t_stream_parser.cc (right):

https://codereview.chromium.org/143973009/diff/1/media/mp2t/mp2t_stream_parse...
media/mp2t/mp2t_stream_parser.cc:171: bool /* ignore_text */ ,
ditto

https://codereview.chromium.org/143973009/diff/1/media/mp2t/mp2t_stream_parser.h
File media/mp2t/mp2t_stream_parser.h (right):

https://codereview.chromium.org/143973009/diff/1/media/mp2t/mp2t_stream_parse...
media/mp2t/mp2t_stream_parser.h:36: bool ignore_text,
nit: s/text/text_tracks/

https://codereview.chromium.org/143973009/diff/1/media/mp3/mp3_stream_parser.cc
File media/mp3/mp3_stream_parser.cc (right):

https://codereview.chromium.org/143973009/diff/1/media/mp3/mp3_stream_parser....
media/mp3/mp3_stream_parser.cc:121: bool ignore_text,
ditto

https://codereview.chromium.org/143973009/diff/1/media/mp3/mp3_stream_parser.h
File media/mp3/mp3_stream_parser.h (right):

https://codereview.chromium.org/143973009/diff/1/media/mp3/mp3_stream_parser....
media/mp3/mp3_stream_parser.h:31: bool ignore_text,
ditto

https://codereview.chromium.org/143973009/diff/1/media/mp4/mp4_stream_parser.h
File media/mp4/mp4_stream_parser.h (right):

https://codereview.chromium.org/143973009/diff/1/media/mp4/mp4_stream_parser....
media/mp4/mp4_stream_parser.h:33: bool ignore_text,
ditto here and elsewhere

https://codereview.chromium.org/143973009/diff/1/media/webm/webm_stream_parse...
File media/webm/webm_stream_parser.cc (right):

https://codereview.chromium.org/143973009/diff/1/media/webm/webm_stream_parse...
media/webm/webm_stream_parser.cc:271: WebMClusterParser::TextTrackIterator
text_track_iter =
Remove this iterator code and just create a scoped_ptr<TextBufferQueueMap> 
GetTextBuffers() on the cluster parser. That would remove a bunch of unnecessary
code and clean up this method.

[wolenetz, 2014-01-23 03:13:24]

PTAL @ patch set 2. Removal of TextTrackIterator also required a few small
changes to webm_cluster_parser_unittest.cc.

Thanks!

https://codereview.chromium.org/143973009/diff/1/media/filters/chunk_demuxer.cc
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/143973009/diff/1/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:186: // TODO(wolenetz): Enable "sequence" mode
logic including MSE-compliant coded
On 2014/01/23 01:32:49, acolwell_OOO_til_1-29 wrote:
> nit: I don't think you need to add this TODO since you are actively working on
> this right now.

Done.

https://codereview.chromium.org/143973009/diff/1/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:778: DCHECK(!audio_buffers.empty() ||
!video_buffers.empty() || text_map);
On 2014/01/23 01:32:49, acolwell_OOO_til_1-29 wrote:
> nit: s/text_map/(text_map && !text_map->empty())/ and remove the TODO.

Done.

https://codereview.chromium.org/143973009/diff/1/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:835: for
(StreamParser::TextBufferQueueMap::const_iterator itr =
On 2014/01/23 01:32:49, acolwell_OOO_til_1-29 wrote:
> nit: Add an bool all_empty = true; before the loop and DCHECK() it before the
> return to verify that we got at least 1 buffer.

Done.

https://codereview.chromium.org/143973009/diff/1/media/mp2t/mp2t_stream_parse...
File media/mp2t/mp2t_stream_parser.cc (right):

https://codereview.chromium.org/143973009/diff/1/media/mp2t/mp2t_stream_parse...
media/mp2t/mp2t_stream_parser.cc:171: bool /* ignore_text */ ,
On 2014/01/23 01:32:49, acolwell_OOO_til_1-29 wrote:
> ditto

Done.

https://codereview.chromium.org/143973009/diff/1/media/mp2t/mp2t_stream_parser.h
File media/mp2t/mp2t_stream_parser.h (right):

https://codereview.chromium.org/143973009/diff/1/media/mp2t/mp2t_stream_parse...
media/mp2t/mp2t_stream_parser.h:36: bool ignore_text,
On 2014/01/23 01:32:49, acolwell_OOO_til_1-29 wrote:
> nit: s/text/text_tracks/

Done.

https://codereview.chromium.org/143973009/diff/1/media/mp3/mp3_stream_parser.cc
File media/mp3/mp3_stream_parser.cc (right):

https://codereview.chromium.org/143973009/diff/1/media/mp3/mp3_stream_parser....
media/mp3/mp3_stream_parser.cc:121: bool ignore_text,
On 2014/01/23 01:32:49, acolwell_OOO_til_1-29 wrote:
> ditto

Done.

https://codereview.chromium.org/143973009/diff/1/media/mp3/mp3_stream_parser.h
File media/mp3/mp3_stream_parser.h (right):

https://codereview.chromium.org/143973009/diff/1/media/mp3/mp3_stream_parser....
media/mp3/mp3_stream_parser.h:31: bool ignore_text,
On 2014/01/23 01:32:49, acolwell_OOO_til_1-29 wrote:
> ditto

Done.

https://codereview.chromium.org/143973009/diff/1/media/mp4/mp4_stream_parser.h
File media/mp4/mp4_stream_parser.h (right):

https://codereview.chromium.org/143973009/diff/1/media/mp4/mp4_stream_parser....
media/mp4/mp4_stream_parser.h:33: bool ignore_text,
On 2014/01/23 01:32:49, acolwell_OOO_til_1-29 wrote:
> ditto here and elsewhere

Done.

https://codereview.chromium.org/143973009/diff/1/media/webm/webm_stream_parse...
File media/webm/webm_stream_parser.cc (right):

https://codereview.chromium.org/143973009/diff/1/media/webm/webm_stream_parse...
media/webm/webm_stream_parser.cc:271: WebMClusterParser::TextTrackIterator
text_track_iter =
On 2014/01/23 01:32:49, acolwell_OOO_til_1-29 wrote:
> Remove this iterator code and just create a scoped_ptr<TextBufferQueueMap> 
> GetTextBuffers() on the cluster parser. That would remove a bunch of
unnecessary
> code and clean up this method.

Done. I'm not sure why we previously had duplicate BufferQueue typedefs in
stream_parser.h and webm_cluster_parser.h, but I needed to do similar for
TextBufferQueueMap.

[wolenetz, 2014-01-23 18:03:05]

On 2014/01/23 03:13:24, wolenetz wrote:
> PTAL @ patch set 2. Removal of TextTrackIterator also required a few small
> changes to webm_cluster_parser_unittest.cc.
> 
> Thanks!
> 
>
https://codereview.chromium.org/143973009/diff/1/media/filters/chunk_demuxer.cc
> File media/filters/chunk_demuxer.cc (right):
> 
>
https://codereview.chromium.org/143973009/diff/1/media/filters/chunk_demuxer....
> media/filters/chunk_demuxer.cc:186: // TODO(wolenetz): Enable "sequence" mode
> logic including MSE-compliant coded
> On 2014/01/23 01:32:49, acolwell_OOO_til_1-29 wrote:
> > nit: I don't think you need to add this TODO since you are actively working
on
> > this right now.
> 
> Done.
> 
>
https://codereview.chromium.org/143973009/diff/1/media/filters/chunk_demuxer....
> media/filters/chunk_demuxer.cc:778: DCHECK(!audio_buffers.empty() ||
> !video_buffers.empty() || text_map);
> On 2014/01/23 01:32:49, acolwell_OOO_til_1-29 wrote:
> > nit: s/text_map/(text_map && !text_map->empty())/ and remove the TODO.
> 
> Done.
> 
>
https://codereview.chromium.org/143973009/diff/1/media/filters/chunk_demuxer....
> media/filters/chunk_demuxer.cc:835: for
> (StreamParser::TextBufferQueueMap::const_iterator itr =
> On 2014/01/23 01:32:49, acolwell_OOO_til_1-29 wrote:
> > nit: Add an bool all_empty = true; before the loop and DCHECK() it before
the
> > return to verify that we got at least 1 buffer.
> 
> Done.
> 
>
https://codereview.chromium.org/143973009/diff/1/media/mp2t/mp2t_stream_parse...
> File media/mp2t/mp2t_stream_parser.cc (right):
> 
>
https://codereview.chromium.org/143973009/diff/1/media/mp2t/mp2t_stream_parse...
> media/mp2t/mp2t_stream_parser.cc:171: bool /* ignore_text */ ,
> On 2014/01/23 01:32:49, acolwell_OOO_til_1-29 wrote:
> > ditto
> 
> Done.
> 
>
https://codereview.chromium.org/143973009/diff/1/media/mp2t/mp2t_stream_parser.h
> File media/mp2t/mp2t_stream_parser.h (right):
> 
>
https://codereview.chromium.org/143973009/diff/1/media/mp2t/mp2t_stream_parse...
> media/mp2t/mp2t_stream_parser.h:36: bool ignore_text,
> On 2014/01/23 01:32:49, acolwell_OOO_til_1-29 wrote:
> > nit: s/text/text_tracks/
> 
> Done.
> 
>
https://codereview.chromium.org/143973009/diff/1/media/mp3/mp3_stream_parser.cc
> File media/mp3/mp3_stream_parser.cc (right):
> 
>
https://codereview.chromium.org/143973009/diff/1/media/mp3/mp3_stream_parser....
> media/mp3/mp3_stream_parser.cc:121: bool ignore_text,
> On 2014/01/23 01:32:49, acolwell_OOO_til_1-29 wrote:
> > ditto
> 
> Done.
> 
> https://codereview.chromium.org/143973009/diff/1/media/mp3/mp3_stream_parser.h
> File media/mp3/mp3_stream_parser.h (right):
> 
>
https://codereview.chromium.org/143973009/diff/1/media/mp3/mp3_stream_parser....
> media/mp3/mp3_stream_parser.h:31: bool ignore_text,
> On 2014/01/23 01:32:49, acolwell_OOO_til_1-29 wrote:
> > ditto
> 
> Done.
> 
> https://codereview.chromium.org/143973009/diff/1/media/mp4/mp4_stream_parser.h
> File media/mp4/mp4_stream_parser.h (right):
> 
>
https://codereview.chromium.org/143973009/diff/1/media/mp4/mp4_stream_parser....
> media/mp4/mp4_stream_parser.h:33: bool ignore_text,
> On 2014/01/23 01:32:49, acolwell_OOO_til_1-29 wrote:
> > ditto here and elsewhere
> 
> Done.
> 
>
https://codereview.chromium.org/143973009/diff/1/media/webm/webm_stream_parse...
> File media/webm/webm_stream_parser.cc (right):
> 
>
https://codereview.chromium.org/143973009/diff/1/media/webm/webm_stream_parse...
> media/webm/webm_stream_parser.cc:271: WebMClusterParser::TextTrackIterator
> text_track_iter =
> On 2014/01/23 01:32:49, acolwell_OOO_til_1-29 wrote:
> > Remove this iterator code and just create a scoped_ptr<TextBufferQueueMap> 
> > GetTextBuffers() on the cluster parser. That would remove a bunch of
> unnecessary
> > code and clean up this method.
> 
> Done. I'm not sure why we previously had duplicate BufferQueue typedefs in
> stream_parser.h and webm_cluster_parser.h, but I needed to do similar for
> TextBufferQueueMap.

From the bots, it looks like I need to rebase this. I'll have another patch set
(#3) posted shortly with only this rebase as the change.
I'll also swap xhwang@ for acolwell@ as reviewer as discussed offline with both.

[wolenetz, 2014-01-23 18:25:39]

xhwang, PTAL. Versus PS2, PS3 is just a rebase due to MSE stream parsers moved
from media/ to media/formats/. acolwell is unavailable as discussed offline.

[xhwang, 2014-01-23 18:44:15]

This CL is looking good! Please see my comments in PS2.

Also, for people that are not familiar this code, it's not obvious what "SS" is
in the CL title. How about:

SourceBuffer: Coalesce OnNewBuffers() CB to include OnTextBuffers() behavior.

https://codereview.chromium.org/143973009/diff/240001/media/base/stream_parser.h
File media/base/stream_parser.h (right):

https://codereview.chromium.org/143973009/diff/240001/media/base/stream_parse...
media/base/stream_parser.h:31: typedef std::map<int, const BufferQueue>
TextBufferQueueMap;
Add a comment about what the int key is for l.30 and l.31?

https://codereview.chromium.org/143973009/diff/240001/media/base/stream_parse...
media/base/stream_parser.h:80: // Initialize the parser with necessary
callbacks. Must be called before any
nit: Initialize_s_

https://codereview.chromium.org/143973009/diff/240001/media/base/stream_parse...
media/base/stream_parser.h:88: bool ignore_text,
ignore_text_tracks to be consistent with Mp2tStreamParser?

https://codereview.chromium.org/143973009/diff/240001/media/filters/chunk_dem...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/143973009/diff/240001/media/filters/chunk_dem...
media/filters/chunk_demuxer.cc:833: text_map->begin(); itr != text_map->end();
++itr) {
Is this indentation done by clang-format? If yes, ignore my comment. Otherwise,
it looks odd to me :)

https://codereview.chromium.org/143973009/diff/240001/media/filters/chunk_dem...
media/filters/chunk_demuxer.cc:837: if (!OnTextBuffers(itr->first,
text_buffers))
So this function doesn't actually take the ownership of |text_map|. This makes
me wonder whether scoped_ptr is the best option. It seems to me it's making the
code more complicated, e.g.
1) We use scoped_ptr for text, but use const-ref for audio/video
2) We have the check of (text_map && !text_map->empty()) in a lot of places.

How about just pass by const-ref?

https://codereview.chromium.org/143973009/diff/240001/media/webm/webm_cluster...
File media/webm/webm_cluster_parser.cc (right):

https://codereview.chromium.org/143973009/diff/240001/media/webm/webm_cluster...
media/webm/webm_cluster_parser.cc:114: result->insert(std::make_pair(itr->first,
text_buffers));
Since there's no duplication in itr->first, how about just do:

result[itr->first] = text_buffers;

https://codereview.chromium.org/143973009/diff/240001/media/webm/webm_cluster...
File media/webm/webm_cluster_parser.h (right):

https://codereview.chromium.org/143973009/diff/240001/media/webm/webm_cluster...
media/webm/webm_cluster_parser.h:81: scoped_ptr<TextBufferQueueMap>
GetTextBuffers() const;
ditto about using scoped_ptr

https://codereview.chromium.org/143973009/diff/240001/media/webm/webm_cluster...
File media/webm/webm_cluster_parser_unittest.cc (right):

https://codereview.chromium.org/143973009/diff/240001/media/webm/webm_cluster...
media/webm/webm_cluster_parser_unittest.cc:476: if (text_map &&
!text_map->empty()) {
ditto: If GetTextBuffers() returns a reference to the text map (which could be
empty), we don't need this check.

[xhwang, 2014-01-23 18:50:53]

rebase in PS3 looks fine :)

[wolenetz, 2014-01-23 21:42:59]

PTAL @ PS4. Thanks!

https://codereview.chromium.org/143973009/diff/240001/media/base/stream_parser.h
File media/base/stream_parser.h (right):

https://codereview.chromium.org/143973009/diff/240001/media/base/stream_parse...
media/base/stream_parser.h:31: typedef std::map<int, const BufferQueue>
TextBufferQueueMap;
On 2014/01/23 18:44:16, xhwang wrote:
> Add a comment about what the int key is for l.30 and l.31?

Done.

https://codereview.chromium.org/143973009/diff/240001/media/base/stream_parse...
media/base/stream_parser.h:80: // Initialize the parser with necessary
callbacks. Must be called before any
On 2014/01/23 18:44:16, xhwang wrote:
> nit: Initialize_s_

Done.

https://codereview.chromium.org/143973009/diff/240001/media/base/stream_parse...
media/base/stream_parser.h:88: bool ignore_text,
On 2014/01/23 18:44:16, xhwang wrote:
> ignore_text_tracks to be consistent with Mp2tStreamParser?

Oops! Done.

https://codereview.chromium.org/143973009/diff/240001/media/filters/chunk_dem...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/143973009/diff/240001/media/filters/chunk_dem...
media/filters/chunk_demuxer.cc:833: text_map->begin(); itr != text_map->end();
++itr) {
On 2014/01/23 18:44:16, xhwang wrote:
> Is this indentation done by clang-format? If yes, ignore my comment.
Otherwise,
> it looks odd to me :)

My fault: bad wrap and bad indentation.
Done now using clang-format style.

https://codereview.chromium.org/143973009/diff/240001/media/filters/chunk_dem...
media/filters/chunk_demuxer.cc:837: if (!OnTextBuffers(itr->first,
text_buffers))
On 2014/01/23 18:44:16, xhwang wrote:
> So this function doesn't actually take the ownership of |text_map|. This makes
> me wonder whether scoped_ptr is the best option. It seems to me it's making
the
> code more complicated, e.g.
> 1) We use scoped_ptr for text, but use const-ref for audio/video
> 2) We have the check of (text_map && !text_map->empty()) in a lot of places.
> 
> How about just pass by const-ref?

Fair point. I had been thinking to reduce the cost of empty std::map
allocation/construction for text for parsers that frequently may be ignoring
text tracks. scoped_ptr makes documentation clear in this case that the receiver
(this function) needs to either retain ownership of the map or delete it. Longer
term in the coded-frame-processing refactoring/implementation, this will matter
less as the interface to OnNewBuffers will likely change to just a single
const-ref queue of muxed-in-decode-timestamp-sequence
annotated-by-type-and-track-id buffers produced by the parsers.

I've reworked this using const-ref. Note that the DCHECKs are altered too to
still require that at least one buffer of some type (a/v/text) is passed to
OnNewBuffers().

https://codereview.chromium.org/143973009/diff/240001/media/webm/webm_cluster...
File media/webm/webm_cluster_parser.cc (right):

https://codereview.chromium.org/143973009/diff/240001/media/webm/webm_cluster...
media/webm/webm_cluster_parser.cc:114: result->insert(std::make_pair(itr->first,
text_buffers));
On 2014/01/23 18:44:16, xhwang wrote:
> Since there's no duplication in itr->first, how about just do:
> 
> result[itr->first] = text_buffers;

Though I agree it looks simpler, it fails to compile since the value type of the
map is const and the operators for this type of implicit insertion/(replacement)
are not const. I think this is because destruction of a pre-existing value is
attempted on insertion collision with this approach.

https://codereview.chromium.org/143973009/diff/240001/media/webm/webm_cluster...
File media/webm/webm_cluster_parser.h (right):

https://codereview.chromium.org/143973009/diff/240001/media/webm/webm_cluster...
media/webm/webm_cluster_parser.h:81: scoped_ptr<TextBufferQueueMap>
GetTextBuffers() const;
On 2014/01/23 18:44:16, xhwang wrote:
> ditto about using scoped_ptr

Done.

https://codereview.chromium.org/143973009/diff/240001/media/webm/webm_cluster...
File media/webm/webm_cluster_parser_unittest.cc (right):

https://codereview.chromium.org/143973009/diff/240001/media/webm/webm_cluster...
media/webm/webm_cluster_parser_unittest.cc:476: if (text_map &&
!text_map->empty()) {
On 2014/01/23 18:44:16, xhwang wrote:
> ditto: If GetTextBuffers() returns a reference to the text map (which could be
> empty), we don't need this check.

Done.

[xhwang, 2014-01-23 22:54:51]

lgtm, thanks!

[commit-bot: I haz the power, 2014-01-23 23:25:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wolenetz@chromium.org/143973009/550002

[commit-bot: I haz the power, 2014-01-24 01:22:26]

Change committed as 246762