Media Source dispatches inband text tracks

Media Source dispatches inband text tracks
    
The Media Source (WebM) parser now detects the presence of inband text
tracks. As frames of inband text (WebVTT cues) are found in the
network stream, they are pushed up the media stack.
    
BUG=230708
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=201716

[Matthew Heaney (Chromium), 2013-04-02 01:42:04]

First draft of changes need to support inband text tracks.

[fgalligan1, 2013-04-02 19:38:32]

This CL probably should have had some text saying that the current block is that
webinbandtexttrackprivateclient_impl.cc will not compile if #include
"third_party/WebKit/Source/WebCore/platform/graphics/InbandTextTrackPrivateClient.h"
is uncommented.

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

https://codereview.chromium.org/13419002/diff/1/media/filters/chunk_demuxer.h...
media/filters/chunk_demuxer.h:39: // TODO(matthewjheaney): temp hack to test cue
rendering
What is the TODO? Remove temp hack to test cue rendering?

https://codereview.chromium.org/13419002/diff/1/webkit/media/webinbandtexttra...
File webkit/media/webinbandtexttrack_impl.cc (right):

https://codereview.chromium.org/13419002/diff/1/webkit/media/webinbandtexttra...
webkit/media/webinbandtexttrack_impl.cc:45: // TODO(matthewjheaney): What is
correct value here?
This will be based on the kind. If kind == captions then this is true. If kind
== subtitles, I think this should be true, but we should check. All other kinds
this should be false.

https://codereview.chromium.org/13419002/diff/1/webkit/media/webinbandtexttra...
webkit/media/webinbandtexttrack_impl.cc:58: // TODO(matthewjheaney): What is
correct value here?
We didn't define "default" track for WebM here
http://wiki.webmproject.org/webm-metadata/temporal-metadata/webvtt-in-webm. We
could add text about FlagDefault element wrt WebM.

https://codereview.chromium.org/13419002/diff/1/webkit/media/webinbandtexttra...
File webkit/media/webinbandtexttrack_impl.h (right):

https://codereview.chromium.org/13419002/diff/1/webkit/media/webinbandtexttra...
webkit/media/webinbandtexttrack_impl.h:8: // Pipeline would creates multiple
threads and access some public methods
s/would//

https://codereview.chromium.org/13419002/diff/1/webkit/media/webinbandtexttra...
webkit/media/webinbandtexttrack_impl.h:14: // destructed nicely, and there will
be some dangling media threads trying to
s/to/to access/

https://codereview.chromium.org/13419002/diff/1/webkit/media/webinbandtexttra...
webkit/media/webinbandtexttrack_impl.h:16: // main thread and cleanup the media
threads when the even is received. Also
s/even/event/

https://codereview.chromium.org/13419002/diff/1/webkit/media/webinbandtexttra...
File webkit/media/webinbandtexttrackprivateclient_impl.cc (right):

https://codereview.chromium.org/13419002/diff/1/webkit/media/webinbandtexttra...
webkit/media/webinbandtexttrackprivateclient_impl.cc:28:
//client_->addWebVTTCue(start,
I think this should be client()->addWebVTTCue()

https://codereview.chromium.org/13419002/diff/1/webkit/media/webinbandtexttra...
File webkit/media/webinbandtexttrackprivateclient_impl.h (right):

https://codereview.chromium.org/13419002/diff/1/webkit/media/webinbandtexttra...
webkit/media/webinbandtexttrackprivateclient_impl.h:44:
WebCore::InbandTextTrackPrivateClient* client_;
Why is this declared here? When it is declared int he base class here
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://codereview.chromium.org/13419002/diff/1/webkit/media/webinbandtexttra...
webkit/media/webinbandtexttrackprivateclient_impl.h:49: #endif
// WEBKIT_MEDIA_WEBINANDTEXTTRACKPRIVATECLIENT_IMPL_H_

https://codereview.chromium.org/13419002/diff/1/webkit/media/webmediaplayer_i...
File webkit/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/13419002/diff/1/webkit/media/webmediaplayer_i...
webkit/media/webmediaplayer_impl.cc:1050: {
Move curly brace up one line.

[scherkus (not reviewing), 2013-04-04 01:03:46]

thanks for getting this uploaded

I'll defer to acolwell to the chunk demuxer / webm parsing code.

it looks like we could probably split this into a few CLs:
  - webkit <-> chromium plumbing
  - webm parsing code
  - chunk demuxer callback stuff

https://codereview.chromium.org/13419002/diff/1/webkit/media/webinbandtexttra...
File webkit/media/webinbandtexttrack_impl.h (right):

https://codereview.chromium.org/13419002/diff/1/webkit/media/webinbandtexttra...
webkit/media/webinbandtexttrack_impl.h:38: //virtual void
setClient(WebCore::InbandTextTrackPrivateClient* client);
you'll have to define a WebKit::WebInbandTextTrackClient type that wraps the
WebCore equivalent

(chromium code can't depend on webcore)

https://codereview.chromium.org/13419002/diff/1/webkit/media/webinbandtexttra...
webkit/media/webinbandtexttrack_impl.h:42: virtual void
setClient(WebKit::WebInbandTextTrackPrivateClient* client);
ah! this looks just about right :)

you can ditch the "Private" bit from the name

[Matthew Heaney (Chromium), 2013-04-04 04:01:52]

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

https://codereview.chromium.org/13419002/diff/1/media/filters/chunk_demuxer.h...
media/filters/chunk_demuxer.h:39: // TODO(matthewjheaney): temp hack to test cue
rendering
Augmented the comment.

https://codereview.chromium.org/13419002/diff/1/webkit/media/webinbandtexttra...
File webkit/media/webinbandtexttrack_impl.cc (right):

https://codereview.chromium.org/13419002/diff/1/webkit/media/webinbandtexttra...
webkit/media/webinbandtexttrack_impl.cc:45: // TODO(matthewjheaney): What is
correct value here?
On 2013/04/02 19:38:32, fgalligan1 wrote:
> This will be based on the kind. If kind == captions then this is true. If kind
> == subtitles, I think this should be true, but we should check. All other
kinds
> this should be false.

Done.

https://codereview.chromium.org/13419002/diff/1/webkit/media/webinbandtexttra...
webkit/media/webinbandtexttrack_impl.cc:45: // TODO(matthewjheaney): What is
correct value here?
On 2013/04/02 19:38:32, fgalligan1 wrote:
> This will be based on the kind. If kind == captions then this is true. If kind
> == subtitles, I think this should be true, but we should check. All other
kinds
> this should be false.

Done.

https://codereview.chromium.org/13419002/diff/1/webkit/media/webinbandtexttra...
webkit/media/webinbandtexttrack_impl.cc:58: // TODO(matthewjheaney): What is
correct value here?
ack

https://codereview.chromium.org/13419002/diff/1/webkit/media/webinbandtexttra...
File webkit/media/webinbandtexttrack_impl.h (right):

https://codereview.chromium.org/13419002/diff/1/webkit/media/webinbandtexttra...
webkit/media/webinbandtexttrack_impl.h:8: // Pipeline would creates multiple
threads and access some public methods
This comment was a copy-and-paste of another.  Comment removed until we resolve
whether any of this is correct.

https://codereview.chromium.org/13419002/diff/1/webkit/media/webinbandtexttra...
webkit/media/webinbandtexttrack_impl.h:14: // destructed nicely, and there will
be some dangling media threads trying to
ack

https://codereview.chromium.org/13419002/diff/1/webkit/media/webinbandtexttra...
webkit/media/webinbandtexttrack_impl.h:16: // main thread and cleanup the media
threads when the even is received. Also
ack

https://codereview.chromium.org/13419002/diff/1/webkit/media/webinbandtexttra...
webkit/media/webinbandtexttrack_impl.h:38: //virtual void
setClient(WebCore::InbandTextTrackPrivateClient* client);
OK.  Have WebKit::WebInbandTextTrackPrivateClient class.

https://codereview.chromium.org/13419002/diff/1/webkit/media/webinbandtexttra...
webkit/media/webinbandtexttrack_impl.h:42: virtual void
setClient(WebKit::WebInbandTextTrackPrivateClient* client);
On 2013/04/04 01:03:46, scherkus wrote:
> ah! this looks just about right :)
> 
> you can ditch the "Private" bit from the name

Done.

https://codereview.chromium.org/13419002/diff/1/webkit/media/webinbandtexttra...
File webkit/media/webinbandtexttrackprivateclient_impl.cc (right):

https://codereview.chromium.org/13419002/diff/1/webkit/media/webinbandtexttra...
webkit/media/webinbandtexttrackprivateclient_impl.cc:28:
//client_->addWebVTTCue(start,
OBE

https://codereview.chromium.org/13419002/diff/1/webkit/media/webinbandtexttra...
File webkit/media/webinbandtexttrackprivateclient_impl.h (right):

https://codereview.chromium.org/13419002/diff/1/webkit/media/webinbandtexttra...
webkit/media/webinbandtexttrackprivateclient_impl.h:44:
WebCore::InbandTextTrackPrivateClient* client_;
OBE

https://codereview.chromium.org/13419002/diff/1/webkit/media/webmediaplayer_i...
File webkit/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/13419002/diff/1/webkit/media/webmediaplayer_i...
webkit/media/webmediaplayer_impl.cc:1050: {
On 2013/04/02 19:38:32, fgalligan1 wrote:
> Move curly brace up one line.

Done.

[acolwell GONE FROM CHROMIUM, 2013-04-05 16:29:23]

This looks like a good start. I agree with Andrew that this should be split up
into multiple patches.

Please update your Chromium checkout so it is using Blink instead of WebKit and
then upload the necessary Blink changes so that I can see what changed there.
You should be able to just rename your existing third_party/WebKit directory to
something else and then run gclient sync. This will checkout Blink into
third_party/WebKit and you should be able to reapply your WebKit patches to the
new WebKit directory. Once you've done this I believe you should be able to use
'git cl upload' from third_party/WebKit to upload the changes to Rietveld

I think the changes should land in the following order:
- Blink changes
- media/ changes
- webkit_media/ changes

https://codereview.chromium.org/13419002/diff/12001/media/base/pipeline_status.h
File media/base/pipeline_status.h (right):

https://codereview.chromium.org/13419002/diff/12001/media/base/pipeline_statu...
media/base/pipeline_status.h:64: enum TextKind {
nit: This doesn't seem like the right home for this especially since the
pipeline isn't even involved in any of these changes. I think this should be put
in something like media/base/text_track.h

https://codereview.chromium.org/13419002/diff/12001/media/base/pipeline_statu...
media/base/pipeline_status.h:76: const std::string& language)> TextTrackCB;
nit:s/TextTrackCB/AddTextTrackCB

It seems like this method should return a callback or object for adding cues and
allowing the track to be removed. If you do this, I think you can avoid having
to pass IDs through this interface.

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

https://codereview.chromium.org/13419002/diff/12001/media/base/stream_parser....
media/base/stream_parser.h:82: const TextTrackCB& text_track_cb,
This likely needs to be a different type. I this callback should return the
NewBuffersCB so that multiple text tracks can be handled naturally w/o the need
for IDs.

https://codereview.chromium.org/13419002/diff/12001/media/filters/chunk_demux...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/13419002/diff/12001/media/filters/chunk_demux...
media/filters/chunk_demuxer.cc:574: text_cb =
base::Bind(&ChunkDemuxer::OnTextBuffers,
nit: Inline this in the Init() call like the other Binds.

https://codereview.chromium.org/13419002/diff/12001/media/filters/chunk_demux...
media/filters/chunk_demuxer.cc:598: void ChunkDemuxer::RemoveId(const
std::string& id) {
You'll need to add code in here to handle the case where the id that is
providing the text tracks gets removed. This should remove the tracks from the
presentation.

https://codereview.chromium.org/13419002/diff/12001/media/filters/chunk_demux...
media/filters/chunk_demuxer.cc:1097: const
StreamParser::BufferQueue::value_type& buffer = *it++;
nit: Please don't use this construct. Use a normal for loop. Doing this makes
the code unnecessarily verbose makes it less clear what is going on here.

https://codereview.chromium.org/13419002/diff/12001/media/filters/chunk_demux...
media/filters/chunk_demuxer.cc:1098: const base::TimeDelta time =
buffer->GetDecodeTimestamp();
AdjustBufferTimestamp() needs to be called here like in the other OnXXXBuffers()
methods.

https://codereview.chromium.org/13419002/diff/12001/media/filters/chunk_demux...
File media/filters/chunk_demuxer.h (right):

https://codereview.chromium.org/13419002/diff/12001/media/filters/chunk_demux...
media/filters/chunk_demuxer.h:55: const TextCB& text_cb,
I think this should be removed and the NewTextTrackCB provides the way to route
cues to the proper location.

https://codereview.chromium.org/13419002/diff/12001/media/webm/webm_stream_pa...
File media/webm/webm_stream_parser.cc (right):

https://codereview.chromium.org/13419002/diff/12001/media/webm/webm_stream_pa...
media/webm/webm_stream_parser.cc:209: text_track_cb_.Run(kTextSubtitles, "",
"");
If text_track_cb_ returns a NewBuffersCB then you should be able to simply
resolve this w/ a std::map<int, NewBuffersCB>.

https://codereview.chromium.org/13419002/diff/12001/webkit/media/webinbandtex...
File webkit/media/webinbandtexttrack_impl.cc (right):

https://codereview.chromium.org/13419002/diff/12001/webkit/media/webinbandtex...
webkit/media/webinbandtexttrack_impl.cc:21: }
Add DCHECK(!client_); and make sure that the WebKit code calls setClient(0)
before releasing its reference. This should clarify the ownership relationship.

https://codereview.chromium.org/13419002/diff/12001/webkit/media/webinbandtex...
webkit/media/webinbandtexttrack_impl.cc:69: return 0;
This appears to be an index for the track order in the underlying media file.
You should probably just pass this value in via the constructor and keep a
counter in the object that creates these objects.

https://codereview.chromium.org/13419002/diff/12001/webkit/media/webmediaplay...
File webkit/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/13419002/diff/12001/webkit/media/webmediaplay...
webkit/media/webmediaplayer_impl.cc:1058: const WebInbandTextTrackImpl::Kind
webkind = kind_map[kind];
A simple cast should be possible here. You can also use a
COMPILE_ASSERT_MATCHING_ENUM() like the one above to make sure the enums always
stay in sync.

https://codereview.chromium.org/13419002/diff/12001/webkit/media/webmediaplay...
File webkit/media/webmediaplayer_impl.h (right):

https://codereview.chromium.org/13419002/diff/12001/webkit/media/webmediaplay...
webkit/media/webmediaplayer_impl.h:371: scoped_refptr<WebInbandTextTrackImpl>
text_track_;
I think some crafty Bind()ing can remove the need for this.

[Matthew Heaney (Chromium), 2013-05-08 19:53:00]

I need some clarification about the suggested changes to the callbacks for
handling text tracks.

https://codereview.chromium.org/13419002/diff/12001/media/base/pipeline_status.h
File media/base/pipeline_status.h (right):

https://codereview.chromium.org/13419002/diff/12001/media/base/pipeline_statu...
media/base/pipeline_status.h:64: enum TextKind {
On 2013/04/05 16:29:23, acolwell wrote:
> nit: This doesn't seem like the right home for this especially since the
> pipeline isn't even involved in any of these changes. I think this should be
put
> in something like media/base/text_track.h

Done.

https://codereview.chromium.org/13419002/diff/12001/media/base/pipeline_statu...
media/base/pipeline_status.h:76: const std::string& language)> TextTrackCB;
Can you elaborate a bit on this?  We're still a little fuzzy here in MTV about
how all the callback stuff works.

Are you saying that the TextTrackCB should change its return type from void to
some other callback type?  (A callback that returns a callback?)  Or that a new
interface should be added?  Or only that chunk_demuxer (or similar) should have
a new method?

[acolwell GONE FROM CHROMIUM, 2013-05-08 20:51:29]

https://codereview.chromium.org/13419002/diff/12001/media/base/pipeline_status.h
File media/base/pipeline_status.h (right):

https://codereview.chromium.org/13419002/diff/12001/media/base/pipeline_statu...
media/base/pipeline_status.h:76: const std::string& language)> TextTrackCB;
On 2013/05/08 19:53:00, Matthew Heaney (Chromium) wrote:
> Can you elaborate a bit on this?  We're still a little fuzzy here in MTV about
> how all the callback stuff works.
> 
> Are you saying that the TextTrackCB should change its return type from void to
> some other callback type?  (A callback that returns a callback?)  Or that a
new
> interface should be added?  Or only that chunk_demuxer (or similar) should
have
> a new method?
Sure. At this point I think the return type should be something along the lines
of scoped_ptr<TextTrack> where TextTrack is declared along these lines:

class TextTrack {
  virtual ~TextTrack() {}
  virtual void addCue(const base::TimeDelta& start, const base::TimeDelta& end,
const std::string& id, const std::string& content, const std::string& settings)
= 0;
}

This basically mirrors the WebInbandTextTrackClient interface you added. When
this object gets destroyed it should remove the track from the presentation.

The abstract TextTrack declaration should be declared in
media/base/text_track.h, but the concrete implementation of this interface
should be in webkit/media/ since it will likely hold a reference to a
WebKit::WebInbandTextTrackClient and you can't reference Blink interfaces from
media/ code.

[Matthew Heaney (Chromium), 2013-05-09 03:53:11]

Please review the latest batch of changes.

I changed the callback mechanism per the sketch Aaron outlined in his comments. 
I had to interpret a few things, so I'm sure I got a few things wrong.

There also remain a few issues related to ptr lifetimes, but I think those will
get resolved as we converge toward's Aaron's callback ideas.

Thanks!

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

https://codereview.chromium.org/13419002/diff/12001/media/base/stream_parser....
media/base/stream_parser.h:82: const TextTrackCB& text_track_cb,
I changed this, using the outline you provided.  See if my changes are what you
had in mind.

https://codereview.chromium.org/13419002/diff/12001/media/filters/chunk_demux...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/13419002/diff/12001/media/filters/chunk_demux...
media/filters/chunk_demuxer.cc:574: text_cb =
base::Bind(&ChunkDemuxer::OnTextBuffers,
I wasn't sure whether this is OBE or not.  Some of this moved into the
webm_stream_parser.  I had to interpret your outline a bit, so see if this
implementation is what you had in mind.

https://codereview.chromium.org/13419002/diff/12001/media/filters/chunk_demux...
media/filters/chunk_demuxer.cc:1097: const
StreamParser::BufferQueue::value_type& buffer = *it++;
On 2013/04/05 16:29:23, acolwell wrote:
> nit: Please don't use this construct. Use a normal for loop. Doing this makes
> the code unnecessarily verbose makes it less clear what is going on here.

Done.

https://codereview.chromium.org/13419002/diff/12001/media/filters/chunk_demux...
media/filters/chunk_demuxer.cc:1098: const base::TimeDelta time =
buffer->GetDecodeTimestamp();
I did have a question about that, since some of this code moved into
webm_stream_parser.cc.  Did I push things too far down?

https://codereview.chromium.org/13419002/diff/12001/media/filters/chunk_demux...
File media/filters/chunk_demuxer.h (right):

https://codereview.chromium.org/13419002/diff/12001/media/filters/chunk_demux...
media/filters/chunk_demuxer.h:55: const TextCB& text_cb,
I changed this per your sketch of the mechanism.  See if the changes are what
you had in mind.

https://codereview.chromium.org/13419002/diff/12001/media/webm/webm_stream_pa...
File media/webm/webm_stream_parser.cc (right):

https://codereview.chromium.org/13419002/diff/12001/media/webm/webm_stream_pa...
media/webm/webm_stream_parser.cc:209: text_track_cb_.Run(kTextSubtitles, "",
"");
OK, I implemented this roughly as you describe.

https://codereview.chromium.org/13419002/diff/12001/webkit/media/webinbandtex...
File webkit/media/webinbandtexttrack_impl.cc (right):

https://codereview.chromium.org/13419002/diff/12001/webkit/media/webinbandtex...
webkit/media/webinbandtexttrack_impl.cc:21: }
On 2013/04/05 16:29:23, acolwell wrote:
> Add DCHECK(!client_); and make sure that the WebKit code calls setClient(0)
> before releasing its reference. This should clarify the ownership
relationship.

Done.

https://codereview.chromium.org/13419002/diff/12001/webkit/media/webinbandtex...
webkit/media/webinbandtexttrack_impl.cc:69: return 0;
On 2013/04/05 16:29:23, acolwell wrote:
> This appears to be an index for the track order in the underlying media file.
> You should probably just pass this value in via the constructor and keep a
> counter in the object that creates these objects.

Done.

https://codereview.chromium.org/13419002/diff/12001/webkit/media/webmediaplay...
File webkit/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/13419002/diff/12001/webkit/media/webmediaplay...
webkit/media/webmediaplayer_impl.cc:1058: const WebInbandTextTrackImpl::Kind
webkind = kind_map[kind];
For now I just made it into a static_cast.

https://codereview.chromium.org/13419002/diff/12001/webkit/media/webmediaplay...
File webkit/media/webmediaplayer_impl.h (right):

https://codereview.chromium.org/13419002/diff/12001/webkit/media/webmediaplay...
webkit/media/webmediaplayer_impl.h:371: scoped_refptr<WebInbandTextTrackImpl>
text_track_;
This might be OBE.  This is now a set, but it remains to be seen whether I've
got things exactly right.

[Matthew Heaney (Chromium), 2013-05-09 20:00:09]

Tom and I studied the compile problem we were having binding the callback.  We
changed from AsWeakPtr to Unretained and that cleared things up.

This revision cleanly compiles and links, and successfully renders the inband
text track.

[acolwell GONE FROM CHROMIUM, 2013-05-10 02:22:08]

Here is an initial round of comments.

Please also try to clean up as many of the TODOs as possible so that the patch
only shows code that you need. If you have quick questions, IMing me is probably
the fastest path to an answer.

https://codereview.chromium.org/13419002/diff/31001/media/base/text_track.h
File media/base/text_track.h (right):

https://codereview.chromium.org/13419002/diff/31001/media/base/text_track.h#n...
media/base/text_track.h:42: typedef base::Callback<TextTrack*(TextKind kind,
A scoped_ptr should be used since I don't believe we need ref counting.

https://codereview.chromium.org/13419002/diff/31001/media/filters/chunk_demux...
File media/filters/chunk_demuxer.h (right):

https://codereview.chromium.org/13419002/diff/31001/media/filters/chunk_demux...
media/filters/chunk_demuxer.h:45: // |text_track_cb| Run when demuxer detects
the presence of an inband
nit: s/text_track_cb/add_text_track_cb/

https://codereview.chromium.org/13419002/diff/31001/media/webm/webm_stream_pa...
File media/webm/webm_stream_parser.cc (right):

https://codereview.chromium.org/13419002/diff/31001/media/webm/webm_stream_pa...
media/webm/webm_stream_parser.cc:21: class WebVTTParser {
This should be in its own file.

https://codereview.chromium.org/13419002/diff/31001/media/webm/webm_stream_pa...
media/webm/webm_stream_parser.cc:388: TextTrack* const text_cb =
cb_iter->second;
nit: Just inline this in the call below.

https://codereview.chromium.org/13419002/diff/31001/media/webm/webm_stream_pa...
media/webm/webm_stream_parser.cc:409: //AdjustBufferTimestamps(buffers);
This happens in SourceState::OnBuffers so you this code should hook into that
functionality like audio & video does.

https://codereview.chromium.org/13419002/diff/31001/media/webm/webm_stream_pa...
media/webm/webm_stream_parser.cc:419: const base::TimeDelta start =
buffer->GetTimestamp();
nit: Since most of these locals appear to only be used in one place, please just
inline them in the appropriate calls. All the type declaration information
doesn't add any value.

https://codereview.chromium.org/13419002/diff/31001/media/webm/webm_stream_pa...
media/webm/webm_stream_parser.cc:428: parser(&id, &settings, &content);
Seeing as it looks like you could rearrange this code to look like
WebVTTParser(payload, payload_size)(&id, &settings, &content), I think you
should just make this a single 5 param function. The object doesn't really
appear to buy us much. ParseWebVTT(payload, payload_size, &id, &settings,
&content) seems much more straight forward and readable.

https://codereview.chromium.org/13419002/diff/31001/media/webm/webm_stream_pa...
File media/webm/webm_stream_parser.h (right):

https://codereview.chromium.org/13419002/diff/31001/media/webm/webm_stream_pa...
media/webm/webm_stream_parser.h:85: // We also need a ruling about element type:
scoped_refptr?  etc
No need for scoped_refptr. You just need to make sure there is a
STLDeleteValues() (base/stl_util.h) in the destructor.

https://codereview.chromium.org/13419002/diff/31001/webkit/media/webmediaplay...
File webkit/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/13419002/diff/31001/webkit/media/webmediaplay...
webkit/media/webmediaplayer_impl.cc:1015: WebTextTrackImpl* const text_cb = new
WebTextTrackImpl(text_track->client());
I think you need to be careful about ownership here. Does addTextTrack()
transfers ownership of text_track to Blink? If so then you need to be careful
about situations where Blink blows away text_track, but text_cb is still alive.

If ownership is not transfered, then text_cb should probably own text_track so
that when the TextTrack is destroyed text_track is also destroyed.

https://codereview.chromium.org/13419002/diff/31001/webkit/media/webmediaplay...
webkit/media/webmediaplayer_impl.cc:1120:
base::Bind(&WebMediaPlayerImpl::OnTextTrack, base::Unretained(this)),
Yes. this is fine.

https://codereview.chromium.org/13419002/diff/31001/webkit/media/webmediaplay...
File webkit/media/webmediaplayer_impl.h (right):

https://codereview.chromium.org/13419002/diff/31001/webkit/media/webmediaplay...
webkit/media/webmediaplayer_impl.h:217: media::TextTrack*
OnTextTrack(media::TextKind kind,
scoped_ptr<TextTrack>

https://codereview.chromium.org/13419002/diff/31001/webkit/media/webtexttrack...
File webkit/media/webtexttrack_impl.cc (right):

https://codereview.chromium.org/13419002/diff/31001/webkit/media/webtexttrack...
webkit/media/webtexttrack_impl.cc:25: const double startF = start.InSecondsF();
nit: Inline all of this in the addWebVTTCue() call please.

https://codereview.chromium.org/13419002/diff/31001/webkit/media/webtexttrack...
File webkit/media/webtexttrack_impl.h (right):

https://codereview.chromium.org/13419002/diff/31001/webkit/media/webtexttrack...
webkit/media/webtexttrack_impl.h:17: class WebTextTrackImpl : public
media::TextTrack {
nit: No need for Web prefix. TextTrackImpl is fine.

[Matthew Heaney (Chromium), 2013-05-10 05:21:08]

This revision incorporates the most recent comments from Aaron, resolving a few
of the questions we had about ptr types and lifetimes.

https://codereview.chromium.org/13419002/diff/31001/media/base/text_track.h
File media/base/text_track.h (right):

https://codereview.chromium.org/13419002/diff/31001/media/base/text_track.h#n...
media/base/text_track.h:42: typedef base::Callback<TextTrack*(TextKind kind,
On 2013/05/10 02:22:08, acolwell wrote:
> A scoped_ptr should be used since I don't believe we need ref counting.

Done.

https://codereview.chromium.org/13419002/diff/31001/media/filters/chunk_demux...
File media/filters/chunk_demuxer.h (right):

https://codereview.chromium.org/13419002/diff/31001/media/filters/chunk_demux...
media/filters/chunk_demuxer.h:45: // |text_track_cb| Run when demuxer detects
the presence of an inband
On 2013/05/10 02:22:08, acolwell wrote:
> nit: s/text_track_cb/add_text_track_cb/

Done.

https://codereview.chromium.org/13419002/diff/31001/media/webm/webm_stream_pa...
File media/webm/webm_stream_parser.cc (right):

https://codereview.chromium.org/13419002/diff/31001/media/webm/webm_stream_pa...
media/webm/webm_stream_parser.cc:21: class WebVTTParser {
On 2013/05/10 02:22:08, acolwell wrote:
> This should be in its own file.

Done.

https://codereview.chromium.org/13419002/diff/31001/media/webm/webm_stream_pa...
media/webm/webm_stream_parser.cc:388: TextTrack* const text_cb =
cb_iter->second;
On 2013/05/10 02:22:08, acolwell wrote:
> nit: Just inline this in the call below.

Done.

https://codereview.chromium.org/13419002/diff/31001/media/webm/webm_stream_pa...
media/webm/webm_stream_parser.cc:388: TextTrack* const text_cb =
cb_iter->second;
On 2013/05/10 02:22:08, acolwell wrote:
> nit: Just inline this in the call below.

Done.

https://codereview.chromium.org/13419002/diff/31001/media/webm/webm_stream_pa...
media/webm/webm_stream_parser.cc:409: //AdjustBufferTimestamps(buffers);
On 2013/05/10 02:22:08, acolwell wrote:
> This happens in SourceState::OnBuffers so you this code should hook into that
> functionality like audio & video does.

Done.

https://codereview.chromium.org/13419002/diff/31001/media/webm/webm_stream_pa...
media/webm/webm_stream_parser.cc:419: const base::TimeDelta start =
buffer->GetTimestamp();
On 2013/05/10 02:22:08, acolwell wrote:
> nit: Since most of these locals appear to only be used in one place, please
just
> inline them in the appropriate calls. All the type declaration information
> doesn't add any value.

This function was cleaned up a bit because the parser declaration was replaced
with a function call.  I left the start and end timestamps as is.  Take a look
at the new revision and just make a note if you'd like me to also remove the
declaration of start and end.

https://codereview.chromium.org/13419002/diff/31001/media/webm/webm_stream_pa...
media/webm/webm_stream_parser.cc:428: parser(&id, &settings, &content);
On 2013/05/10 02:22:08, acolwell wrote:
> Seeing as it looks like you could rearrange this code to look like
> WebVTTParser(payload, payload_size)(&id, &settings, &content), I think you
> should just make this a single 5 param function. The object doesn't really
> appear to buy us much. ParseWebVTT(payload, payload_size, &id, &settings,
> &content) seems much more straight forward and readable.

I moved the webvtt parser to its own class, and added a static function.

https://codereview.chromium.org/13419002/diff/31001/media/webm/webm_stream_pa...
File media/webm/webm_stream_parser.h (right):

https://codereview.chromium.org/13419002/diff/31001/media/webm/webm_stream_pa...
media/webm/webm_stream_parser.h:85: // We also need a ruling about element type:
scoped_refptr?  etc
On 2013/05/10 02:22:08, acolwell wrote:
> No need for scoped_refptr. You just need to make sure there is a
> STLDeleteValues() (base/stl_util.h) in the destructor.

Done.

https://codereview.chromium.org/13419002/diff/31001/webkit/media/webmediaplay...
File webkit/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/13419002/diff/31001/webkit/media/webmediaplay...
webkit/media/webmediaplayer_impl.cc:1015: WebTextTrackImpl* const text_cb = new
WebTextTrackImpl(text_track->client());
On 2013/05/10 02:22:08, acolwell wrote:
> I think you need to be careful about ownership here. Does addTextTrack()
> transfers ownership of text_track to Blink? If so then you need to be careful
> about situations where Blink blows away text_track, but text_cb is still
alive.
> 
> If ownership is not transfered, then text_cb should probably own text_track so
> that when the TextTrack is destroyed text_track is also destroyed.

OK, that makes sense.  I'll make the necessary changes to webkit and create a
separate CL for that.

https://codereview.chromium.org/13419002/diff/31001/webkit/media/webmediaplay...
webkit/media/webmediaplayer_impl.cc:1120:
base::Bind(&WebMediaPlayerImpl::OnTextTrack, base::Unretained(this)),
On 2013/05/10 02:22:08, acolwell wrote:
> Yes. this is fine.

Done.

https://codereview.chromium.org/13419002/diff/31001/webkit/media/webmediaplay...
File webkit/media/webmediaplayer_impl.h (right):

https://codereview.chromium.org/13419002/diff/31001/webkit/media/webmediaplay...
webkit/media/webmediaplayer_impl.h:217: media::TextTrack*
OnTextTrack(media::TextKind kind,
On 2013/05/10 02:22:08, acolwell wrote:
> scoped_ptr<TextTrack>

Done.

https://codereview.chromium.org/13419002/diff/31001/webkit/media/webtexttrack...
File webkit/media/webtexttrack_impl.cc (right):

https://codereview.chromium.org/13419002/diff/31001/webkit/media/webtexttrack...
webkit/media/webtexttrack_impl.cc:25: const double startF = start.InSecondsF();
On 2013/05/10 02:22:08, acolwell wrote:
> nit: Inline all of this in the addWebVTTCue() call please.

Done.

https://codereview.chromium.org/13419002/diff/31001/webkit/media/webtexttrack...
File webkit/media/webtexttrack_impl.h (right):

https://codereview.chromium.org/13419002/diff/31001/webkit/media/webtexttrack...
webkit/media/webtexttrack_impl.h:17: class WebTextTrackImpl : public
media::TextTrack {
On 2013/05/10 02:22:08, acolwell wrote:
> nit: No need for Web prefix. TextTrackImpl is fine.

Done.

[acolwell GONE FROM CHROMIUM, 2013-05-10 18:41:39]

Getting closer. :) Here are a few more comments.

https://codereview.chromium.org/13419002/diff/43001/media/filters/chunk_demux...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/13419002/diff/43001/media/filters/chunk_demux...
media/filters/chunk_demuxer.cc:716: text_cb =
base::Bind(&ChunkDemuxer::OnTextBuffers,
nit: Please inline like the other Bind calls below.

https://codereview.chromium.org/13419002/diff/43001/media/filters/chunk_demux...
media/filters/chunk_demuxer.cc:1210: // so it doesn't appear that there's
anything we need to do here.
Text tracks need to be able to update the duration like the audio and video
tracks do. See the IncreaseDurationIfNecessary() calls above. I'm fine with
adding a TODO for this and addressing it in a second CL since it might lead to a
yak shave.

You should change the signature of this method to bool(TextTrack*, const
StreamParser::BufferQueue& buffers) and move the
WebMStreamParser::OnTextBuffers() and WebMStreamParser::OnTextBuffer() logic
here. These don't need to be seperate methods since the code is so simple. This
also implies that you'll need to make a method SourceState::OnTextBuffers that
is almost identical to SourceState::OnBuffers, but also forwards the new
TextTrack parameter.

https://codereview.chromium.org/13419002/diff/43001/media/webm/webm_stream_pa...
File media/webm/webm_stream_parser.cc (right):

https://codereview.chromium.org/13419002/diff/43001/media/webm/webm_stream_pa...
media/webm/webm_stream_parser.cc:210: typedef std::set<int> TextTracksSet;
nit: This typedef should probably be in WebMTracksParser and referenced here.

https://codereview.chromium.org/13419002/diff/43001/media/webm/webm_stream_pa...
media/webm/webm_stream_parser.cc:215: // TODO(matthewjheaney): parse track kind,
name, lang from track header
I'd like to see a separate CL with the necessary changes for this to land before
this patch. It looks like we already have most of the parsing in place for kind
at least. I just don't want to enable inline text tracks and assume that they
are all subtitles.

https://codereview.chromium.org/13419002/diff/43001/media/webm/webm_stream_pa...
media/webm/webm_stream_parser.cc:216: scoped_ptr<TextTrack> text_cb =
nit: s/text_cb/text_track/

https://codereview.chromium.org/13419002/diff/43001/media/webm/webm_stream_pa...
media/webm/webm_stream_parser.cc:220: TextTrack* const text_cb_ptr =
text_cb.release();
nit: Just inline please.

https://codereview.chromium.org/13419002/diff/43001/media/webm/webm_stream_pa...
media/webm/webm_stream_parser.cc:300: int tt_num;
nit: s/tt_num/track_id/ or s/tt_num/track_number/

https://codereview.chromium.org/13419002/diff/43001/media/webm/webm_stream_pa...
media/webm/webm_stream_parser.cc:301: const BufferQueue* tt_buffers;
nit: s/tt_buffers/buffers since the tt doesn't really distinguish or clarify
anything.

https://codereview.chromium.org/13419002/diff/43001/media/webm/webm_stream_pa...
media/webm/webm_stream_parser.cc:330: TextTrack* text_cb) {
nit: when this is moved to ChunkDemuxer s/text_cb/text_track/

https://codereview.chromium.org/13419002/diff/43001/media/webm/webm_webvtt_pa...
File media/webm/webm_webvtt_parser.cc (right):

https://codereview.chromium.org/13419002/diff/43001/media/webm/webm_webvtt_pa...
media/webm/webm_webvtt_parser.cc:1: // Copyright (c) 2012 The Chromium Authors.
All rights reserved.
nit: s/2012/2013/

https://codereview.chromium.org/13419002/diff/43001/media/webm/webm_webvtt_pa...
media/webm/webm_webvtt_parser.cc:22: void
WebMWebVTTParser::operator()(std::string* id,
nit: It would be better for readability if this function had a name instead.
Something like ParseBuffer, DoParse, ParseFields, etc.

https://codereview.chromium.org/13419002/diff/43001/media/webm/webm_webvtt_pa...
media/webm/webm_webvtt_parser.cc:43: std::string& line = *line_ptr;
nit: This isn't really necessary. Just use -> below.

https://codereview.chromium.org/13419002/diff/43001/media/webm/webm_webvtt_pa...
File media/webm/webm_webvtt_parser.h (right):

https://codereview.chromium.org/13419002/diff/43001/media/webm/webm_webvtt_pa...
media/webm/webm_webvtt_parser.h:24: WebMWebVTTParser(const uint8* payload, int
payload_size);
Make these private since we don't want external code to call it.

https://codereview.chromium.org/13419002/diff/43001/webkit/media/texttrack_im...
File webkit/media/texttrack_impl.h (right):

https://codereview.chromium.org/13419002/diff/43001/webkit/media/texttrack_im...
webkit/media/texttrack_impl.h:19: explicit
TextTrackImpl(WebKit::WebInbandTextTrackClient*);
nit: Add parameter name here since this is chromium code.

https://codereview.chromium.org/13419002/diff/43001/webkit/media/webmediaplay...
File webkit/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/13419002/diff/43001/webkit/media/webmediaplay...
webkit/media/webmediaplayer_impl.cc:1017: TextTrackImpl* const text_cb = new
TextTrackImpl(text_track->client());
nit: Just inline this below.

[Matthew Heaney (Chromium), 2013-05-11 07:29:12]

Incorporated the latest comments from Aaron.  Some of the text buffer handling
moved back up into SourceState.

https://codereview.chromium.org/13419002/diff/43001/media/filters/chunk_demux...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/13419002/diff/43001/media/filters/chunk_demux...
media/filters/chunk_demuxer.cc:716: text_cb =
base::Bind(&ChunkDemuxer::OnTextBuffers,
On 2013/05/10 18:41:39, acolwell wrote:
> nit: Please inline like the other Bind calls below.

Done.

https://codereview.chromium.org/13419002/diff/43001/media/filters/chunk_demux...
media/filters/chunk_demuxer.cc:1210: // so it doesn't appear that there's
anything we need to do here.
On 2013/05/10 18:41:39, acolwell wrote:
> Text tracks need to be able to update the duration like the audio and video
> tracks do. See the IncreaseDurationIfNecessary() calls above. I'm fine with
> adding a TODO for this and addressing it in a second CL since it might lead to
a
> yak shave.
> 
> You should change the signature of this method to bool(TextTrack*, const
> StreamParser::BufferQueue& buffers) and move the
> WebMStreamParser::OnTextBuffers() and WebMStreamParser::OnTextBuffer() logic
> here. These don't need to be seperate methods since the code is so simple.
This
> also implies that you'll need to make a method SourceState::OnTextBuffers that
> is almost identical to SourceState::OnBuffers, but also forwards the new
> TextTrack parameter.

Done.

https://codereview.chromium.org/13419002/diff/43001/media/webm/webm_stream_pa...
File media/webm/webm_stream_parser.cc (right):

https://codereview.chromium.org/13419002/diff/43001/media/webm/webm_stream_pa...
media/webm/webm_stream_parser.cc:210: typedef std::set<int> TextTracksSet;
On 2013/05/10 18:41:39, acolwell wrote:
> nit: This typedef should probably be in WebMTracksParser and referenced here.

Done.

https://codereview.chromium.org/13419002/diff/43001/media/webm/webm_stream_pa...
media/webm/webm_stream_parser.cc:215: // TODO(matthewjheaney): parse track kind,
name, lang from track header
On 2013/05/10 18:41:39, acolwell wrote:
> I'd like to see a separate CL with the necessary changes for this to land
before
> this patch. It looks like we already have most of the parsing in place for
kind
> at least. I just don't want to enable inline text tracks and assume that they
> are all subtitles.

Done.

https://codereview.chromium.org/13419002/diff/43001/media/webm/webm_stream_pa...
media/webm/webm_stream_parser.cc:216: scoped_ptr<TextTrack> text_cb =
On 2013/05/10 18:41:39, acolwell wrote:
> nit: s/text_cb/text_track/

Done.

https://codereview.chromium.org/13419002/diff/43001/media/webm/webm_stream_pa...
media/webm/webm_stream_parser.cc:216: scoped_ptr<TextTrack> text_cb =
On 2013/05/10 18:41:39, acolwell wrote:
> nit: s/text_cb/text_track/

Done.

https://codereview.chromium.org/13419002/diff/43001/media/webm/webm_stream_pa...
media/webm/webm_stream_parser.cc:220: TextTrack* const text_cb_ptr =
text_cb.release();
On 2013/05/10 18:41:39, acolwell wrote:
> nit: Just inline please.

Done.

https://codereview.chromium.org/13419002/diff/43001/media/webm/webm_stream_pa...
media/webm/webm_stream_parser.cc:220: TextTrack* const text_cb_ptr =
text_cb.release();
On 2013/05/10 18:41:39, acolwell wrote:
> nit: Just inline please.

Done.

https://codereview.chromium.org/13419002/diff/43001/media/webm/webm_stream_pa...
media/webm/webm_stream_parser.cc:300: int tt_num;
On 2013/05/10 18:41:39, acolwell wrote:
> nit: s/tt_num/track_id/ or s/tt_num/track_number/

Done.

https://codereview.chromium.org/13419002/diff/43001/media/webm/webm_stream_pa...
media/webm/webm_stream_parser.cc:301: const BufferQueue* tt_buffers;
On 2013/05/10 18:41:39, acolwell wrote:
> nit: s/tt_buffers/buffers since the tt doesn't really distinguish or clarify
> anything.

Done.

https://codereview.chromium.org/13419002/diff/43001/media/webm/webm_stream_pa...
media/webm/webm_stream_parser.cc:330: TextTrack* text_cb) {
On 2013/05/10 18:41:39, acolwell wrote:
> nit: when this is moved to ChunkDemuxer s/text_cb/text_track/

Done.

https://codereview.chromium.org/13419002/diff/43001/media/webm/webm_webvtt_pa...
File media/webm/webm_webvtt_parser.cc (right):

https://codereview.chromium.org/13419002/diff/43001/media/webm/webm_webvtt_pa...
media/webm/webm_webvtt_parser.cc:1: // Copyright (c) 2012 The Chromium Authors.
All rights reserved.
On 2013/05/10 18:41:39, acolwell wrote:
> nit: s/2012/2013/

Done.

https://codereview.chromium.org/13419002/diff/43001/media/webm/webm_webvtt_pa...
media/webm/webm_webvtt_parser.cc:22: void
WebMWebVTTParser::operator()(std::string* id,
On 2013/05/10 18:41:39, acolwell wrote:
> nit: It would be better for readability if this function had a name instead.
> Something like ParseBuffer, DoParse, ParseFields, etc.

Done.

https://codereview.chromium.org/13419002/diff/43001/media/webm/webm_webvtt_pa...
media/webm/webm_webvtt_parser.cc:43: std::string& line = *line_ptr;
On 2013/05/10 18:41:39, acolwell wrote:
> nit: This isn't really necessary. Just use -> below.

Done.

https://codereview.chromium.org/13419002/diff/43001/media/webm/webm_webvtt_pa...
File media/webm/webm_webvtt_parser.h (right):

https://codereview.chromium.org/13419002/diff/43001/media/webm/webm_webvtt_pa...
media/webm/webm_webvtt_parser.h:24: WebMWebVTTParser(const uint8* payload, int
payload_size);
On 2013/05/10 18:41:39, acolwell wrote:
> Make these private since we don't want external code to call it.

Done.

https://codereview.chromium.org/13419002/diff/43001/webkit/media/texttrack_im...
File webkit/media/texttrack_impl.h (right):

https://codereview.chromium.org/13419002/diff/43001/webkit/media/texttrack_im...
webkit/media/texttrack_impl.h:19: explicit
TextTrackImpl(WebKit::WebInbandTextTrackClient*);
On 2013/05/10 18:41:39, acolwell wrote:
> nit: Add parameter name here since this is chromium code.

Done.

https://codereview.chromium.org/13419002/diff/43001/webkit/media/webmediaplay...
File webkit/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/13419002/diff/43001/webkit/media/webmediaplay...
webkit/media/webmediaplayer_impl.cc:1017: TextTrackImpl* const text_cb = new
TextTrackImpl(text_track->client());
On 2013/05/10 18:41:39, acolwell wrote:
> nit: Just inline this below.

Done.

[Matthew Heaney (Chromium), 2013-05-13 20:26:33]

I just pushed up a delta that adds a pipeline integration unit test.

[Matthew Heaney (Chromium), 2013-05-13 22:07:29]

Added a unit test for the webvtt parser.

[acolwell GONE FROM CHROMIUM, 2013-05-15 22:42:36]

Looks good. Just a few things that need to be resolved before I can give the
final ok.

https://chromiumcodereview.appspot.com/13419002/diff/70001/media/filters/pipe...
File media/filters/pipeline_integration_test.cc (right):

https://chromiumcodereview.appspot.com/13419002/diff/70001/media/filters/pipe...
media/filters/pipeline_integration_test.cc:333: return scoped_ptr<TextTrack>(0);
What are your plans here? At a minimum the caller needs to protect itself from
NULL being returned here. Perhaps have it avoid creating a map entry for this
track?

https://chromiumcodereview.appspot.com/13419002/diff/70001/media/webm/webm_we...
File media/webm/webm_webvtt_parser_unittest.cc (right):

https://chromiumcodereview.appspot.com/13419002/diff/70001/media/webm/webm_we...
media/webm/webm_webvtt_parser_unittest.cc:74: strbuf += "\n";
nit: If you create a helper function along the lines of std::vector<uint8>
EncodeCue(const std::string& id, const std::string& settings, const std::string&
content) I think it would make these tests a little easier to read.

https://chromiumcodereview.appspot.com/13419002/diff/70001/webkit/media/textt...
File webkit/media/texttrack_impl.cc (right):

https://chromiumcodereview.appspot.com/13419002/diff/70001/webkit/media/textt...
webkit/media/texttrack_impl.cc:17: // text track during destruction of this
object.
Is the plan still to do this? This should be resolved before l g t m.

https://chromiumcodereview.appspot.com/13419002/diff/70001/webkit/media/webme...
File webkit/media/webmediaplayer_impl.cc (right):

https://chromiumcodereview.appspot.com/13419002/diff/70001/webkit/media/webme...
webkit/media/webmediaplayer_impl.cc:1009: // its (owning) client.
This needs to be resolved before the final l g t m.

https://chromiumcodereview.appspot.com/13419002/diff/70001/webkit/media/webme...
webkit/media/webmediaplayer_impl.cc:1017: // This means webkit will not adopt
text_track.
ditto

[Matthew Heaney (Chromium), 2013-05-16 22:05:22]

Resolves an issue with respect to lifetime of the WebInbandTextTrack object.

https://chromiumcodereview.appspot.com/13419002/diff/70001/media/filters/pipe...
File media/filters/pipeline_integration_test.cc (right):

https://chromiumcodereview.appspot.com/13419002/diff/70001/media/filters/pipe...
media/filters/pipeline_integration_test.cc:333: return scoped_ptr<TextTrack>(0);
On 2013/05/15 22:42:36, acolwell wrote:
> What are your plans here? At a minimum the caller needs to protect itself from
> NULL being returned here. Perhaps have it avoid creating a map entry for this
> track?

Done.

https://chromiumcodereview.appspot.com/13419002/diff/70001/media/webm/webm_we...
File media/webm/webm_webvtt_parser_unittest.cc (right):

https://chromiumcodereview.appspot.com/13419002/diff/70001/media/webm/webm_we...
media/webm/webm_webvtt_parser_unittest.cc:74: strbuf += "\n";
On 2013/05/15 22:42:36, acolwell wrote:
> nit: If you create a helper function along the lines of std::vector<uint8>
> EncodeCue(const std::string& id, const std::string& settings, const
std::string&
> content) I think it would make these tests a little easier to read.

Done.

https://chromiumcodereview.appspot.com/13419002/diff/70001/webkit/media/textt...
File webkit/media/texttrack_impl.cc (right):

https://chromiumcodereview.appspot.com/13419002/diff/70001/webkit/media/textt...
webkit/media/texttrack_impl.cc:17: // text track during destruction of this
object.
On 2013/05/15 22:42:36, acolwell wrote:
> Is the plan still to do this? This should be resolved before l g t m.

Done.

https://chromiumcodereview.appspot.com/13419002/diff/70001/webkit/media/webme...
File webkit/media/webmediaplayer_impl.cc (right):

https://chromiumcodereview.appspot.com/13419002/diff/70001/webkit/media/webme...
webkit/media/webmediaplayer_impl.cc:1009: // its (owning) client.
On 2013/05/15 22:42:36, acolwell wrote:
> This needs to be resolved before the final l g t m.

Done.

https://chromiumcodereview.appspot.com/13419002/diff/70001/webkit/media/webme...
webkit/media/webmediaplayer_impl.cc:1017: // This means webkit will not adopt
text_track.
On 2013/05/15 22:42:36, acolwell wrote:
> ditto

Done.

[acolwell GONE FROM CHROMIUM, 2013-05-17 16:32:08]

LGTM % nits

You'll have to wait for a Blink DEPS roll before landing this. You definitely
want to make sure that your changes in https://codereview.chromium.org/15236002/
are in place before landing. Once that CL has landed just watch for the
webkit_revision in DEPS to go past the revision that contains you change. Once
it does you should be safe to land this.

Thanks for your patience and hard work on this CL.

https://codereview.chromium.org/13419002/diff/76001/media/filters/chunk_demux...
File media/filters/chunk_demuxer_unittest.cc (right):

https://codereview.chromium.org/13419002/diff/76001/media/filters/chunk_demux...
media/filters/chunk_demuxer_unittest.cc:784: // TODO(matthewjheaney): put
something here
nit: Please make it explict what needs to be done or remove the comment.

https://codereview.chromium.org/13419002/diff/76001/media/filters/chunk_demux...
media/filters/chunk_demuxer_unittest.cc:785: return scoped_ptr<TextTrack>(0);
nit: You shouldn't need the 0.

https://codereview.chromium.org/13419002/diff/76001/media/filters/pipeline_in...
File media/filters/pipeline_integration_test.cc (right):

https://codereview.chromium.org/13419002/diff/76001/media/filters/pipeline_in...
media/filters/pipeline_integration_test.cc:332: return scoped_ptr<TextTrack>(0);
nit: You shouldn't need the 0.

https://codereview.chromium.org/13419002/diff/76001/media/webm/webm_stream_pa...
File media/webm/webm_stream_parser.cc (right):

https://codereview.chromium.org/13419002/diff/76001/media/webm/webm_stream_pa...
media/webm/webm_stream_parser.cc:226: if (text_track.get())
nit: I don't think you need the .get()

https://codereview.chromium.org/13419002/diff/76001/media/webm/webm_stream_pa...
File media/webm/webm_stream_parser.h (right):

https://codereview.chromium.org/13419002/diff/76001/media/webm/webm_stream_pa...
media/webm/webm_stream_parser.h:79: typedef std::map<int, TextTrack* >
TextTrackMap;
nit: Add include for <map>

https://codereview.chromium.org/13419002/diff/76001/media/webm/webm_webvtt_pa...
File media/webm/webm_webvtt_parser_unittest.cc (right):

https://codereview.chromium.org/13419002/diff/76001/media/webm/webm_webvtt_pa...
media/webm/webm_webvtt_parser_unittest.cc:5: //#include "base/logging.h"
nit: Remove

https://codereview.chromium.org/13419002/diff/76001/media/webm/webm_webvtt_pa...
media/webm/webm_webvtt_parser_unittest.cc:11: using ::testing::Return;
nit: I don't think this or the line below is needed.

https://codereview.chromium.org/13419002/diff/76001/media/webm/webm_webvtt_pa...
media/webm/webm_webvtt_parser_unittest.cc:24: const Cue::size_type buflen =
Cue::size_type(result.length());
nit: You should just use "buf + result.length()" in the line below. It is way
less text than this.

https://codereview.chromium.org/13419002/diff/76001/media/webm/webm_webvtt_pa...
media/webm/webm_webvtt_parser_unittest.cc:33: const uint8* const buf = &cue[0];
nit: Inline these.

https://codereview.chromium.org/13419002/diff/76001/media/webm/webm_webvtt_pa...
media/webm/webm_webvtt_parser_unittest.cc:59: const char c = '0' + i;
nit: inline

https://codereview.chromium.org/13419002/diff/76001/media/webm/webm_webvtt_pa...
media/webm/webm_webvtt_parser_unittest.cc:67: EXPECT_EQ(id[0], c);
nit: Just compare id to idsrc instead of using these two lines.

https://codereview.chromium.org/13419002/diff/76001/webkit/media/texttrack_im...
File webkit/media/texttrack_impl.cc (right):

https://codereview.chromium.org/13419002/diff/76001/webkit/media/texttrack_im...
webkit/media/texttrack_impl.cc:13: // This impl object assumes owndership of the
text_track object.
nit:s/owndership/ownership/

https://codereview.chromium.org/13419002/diff/76001/webkit/media/texttrack_im...
webkit/media/texttrack_impl.cc:25:
text_track_->client()->addWebVTTCue(start.InSecondsF(),
nit: I think you may need a null check on client() since I think it might be
possible for the client to get cleared before this object is destroyed.

https://codereview.chromium.org/13419002/diff/76001/webkit/media/texttrack_im...
File webkit/media/texttrack_impl.h (right):

https://codereview.chromium.org/13419002/diff/76001/webkit/media/texttrack_im...
webkit/media/texttrack_impl.h:7: 
nit:Add <string> include.

https://codereview.chromium.org/13419002/diff/76001/webkit/media/texttrack_im...
webkit/media/texttrack_impl.h:27: WebInbandTextTrackImpl* text_track_;
nit: Make scoped_ptr<WebInbandTextTrackImpl>

https://codereview.chromium.org/13419002/diff/76001/webkit/media/webmediaplay...
File webkit/media/webmediaplayer_impl.h (right):

https://codereview.chromium.org/13419002/diff/76001/webkit/media/webmediaplay...
webkit/media/webmediaplayer_impl.h:59: class TextTrack;
nit: I don't think you need this since you are including the decl above.

[Matthew Heaney (Chromium), 2013-05-18 01:35:48]

I incorporated Aaron's comments.

https://codereview.chromium.org/13419002/diff/76001/media/filters/chunk_demux...
File media/filters/chunk_demuxer_unittest.cc (right):

https://codereview.chromium.org/13419002/diff/76001/media/filters/chunk_demux...
media/filters/chunk_demuxer_unittest.cc:784: // TODO(matthewjheaney): put
something here
On 2013/05/17 16:32:08, acolwell wrote:
> nit: Please make it explict what needs to be done or remove the comment.

Done.

https://codereview.chromium.org/13419002/diff/76001/media/filters/chunk_demux...
media/filters/chunk_demuxer_unittest.cc:785: return scoped_ptr<TextTrack>(0);
On 2013/05/17 16:32:08, acolwell wrote:
> nit: You shouldn't need the 0.

Done.

https://codereview.chromium.org/13419002/diff/76001/media/filters/pipeline_in...
File media/filters/pipeline_integration_test.cc (right):

https://codereview.chromium.org/13419002/diff/76001/media/filters/pipeline_in...
media/filters/pipeline_integration_test.cc:332: return scoped_ptr<TextTrack>(0);
On 2013/05/17 16:32:08, acolwell wrote:
> nit: You shouldn't need the 0.

Done.

https://codereview.chromium.org/13419002/diff/76001/media/webm/webm_stream_pa...
File media/webm/webm_stream_parser.cc (right):

https://codereview.chromium.org/13419002/diff/76001/media/webm/webm_stream_pa...
media/webm/webm_stream_parser.cc:226: if (text_track.get())
On 2013/05/17 16:32:08, acolwell wrote:
> nit: I don't think you need the .get()

Done.

https://codereview.chromium.org/13419002/diff/76001/media/webm/webm_stream_pa...
File media/webm/webm_stream_parser.h (right):

https://codereview.chromium.org/13419002/diff/76001/media/webm/webm_stream_pa...
media/webm/webm_stream_parser.h:79: typedef std::map<int, TextTrack* >
TextTrackMap;
On 2013/05/17 16:32:08, acolwell wrote:
> nit: Add include for <map>

Done.

https://codereview.chromium.org/13419002/diff/76001/media/webm/webm_webvtt_pa...
File media/webm/webm_webvtt_parser_unittest.cc (right):

https://codereview.chromium.org/13419002/diff/76001/media/webm/webm_webvtt_pa...
media/webm/webm_webvtt_parser_unittest.cc:5: //#include "base/logging.h"
On 2013/05/17 16:32:08, acolwell wrote:
> nit: Remove

Done.

https://codereview.chromium.org/13419002/diff/76001/media/webm/webm_webvtt_pa...
media/webm/webm_webvtt_parser_unittest.cc:11: using ::testing::Return;
On 2013/05/17 16:32:08, acolwell wrote:
> nit: I don't think this or the line below is needed.

Done.

https://codereview.chromium.org/13419002/diff/76001/media/webm/webm_webvtt_pa...
media/webm/webm_webvtt_parser_unittest.cc:24: const Cue::size_type buflen =
Cue::size_type(result.length());
On 2013/05/17 16:32:08, acolwell wrote:
> nit: You should just use "buf + result.length()" in the line below. It is way
> less text than this.

Done.

https://codereview.chromium.org/13419002/diff/76001/media/webm/webm_webvtt_pa...
media/webm/webm_webvtt_parser_unittest.cc:33: const uint8* const buf = &cue[0];
On 2013/05/17 16:32:08, acolwell wrote:
> nit: Inline these.

Done.

https://codereview.chromium.org/13419002/diff/76001/media/webm/webm_webvtt_pa...
media/webm/webm_webvtt_parser_unittest.cc:59: const char c = '0' + i;
On 2013/05/17 16:32:08, acolwell wrote:
> nit: inline

Done.

https://codereview.chromium.org/13419002/diff/76001/media/webm/webm_webvtt_pa...
media/webm/webm_webvtt_parser_unittest.cc:67: EXPECT_EQ(id[0], c);
On 2013/05/17 16:32:08, acolwell wrote:
> nit: Just compare id to idsrc instead of using these two lines.

Done.

https://codereview.chromium.org/13419002/diff/76001/webkit/media/texttrack_im...
File webkit/media/texttrack_impl.cc (right):

https://codereview.chromium.org/13419002/diff/76001/webkit/media/texttrack_im...
webkit/media/texttrack_impl.cc:13: // This impl object assumes owndership of the
text_track object.
On 2013/05/17 16:32:08, acolwell wrote:
> nit:s/owndership/ownership/ 

Done.

https://codereview.chromium.org/13419002/diff/76001/webkit/media/texttrack_im...
webkit/media/texttrack_impl.cc:25:
text_track_->client()->addWebVTTCue(start.InSecondsF(),
On 2013/05/17 16:32:08, acolwell wrote:
> nit: I think you may need a null check on client() since I think it might be
> possible for the client to get cleared before this object is destroyed.

Done.

https://codereview.chromium.org/13419002/diff/76001/webkit/media/texttrack_im...
File webkit/media/texttrack_impl.h (right):

https://codereview.chromium.org/13419002/diff/76001/webkit/media/texttrack_im...
webkit/media/texttrack_impl.h:7: 
On 2013/05/17 16:32:08, acolwell wrote:
> nit:Add <string> include.

Done.

https://codereview.chromium.org/13419002/diff/76001/webkit/media/texttrack_im...
webkit/media/texttrack_impl.h:27: WebInbandTextTrackImpl* text_track_;
On 2013/05/17 16:32:08, acolwell wrote:
> nit: Make scoped_ptr<WebInbandTextTrackImpl>

Done.

https://codereview.chromium.org/13419002/diff/76001/webkit/media/webmediaplay...
File webkit/media/webmediaplayer_impl.h (right):

https://codereview.chromium.org/13419002/diff/76001/webkit/media/webmediaplay...
webkit/media/webmediaplayer_impl.h:59: class TextTrack;
On 2013/05/17 16:32:08, acolwell wrote:
> nit: I don't think you need this since you are including the decl above.

Done.

[commit-bot: I haz the power, 2013-05-21 19:08:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/matthewjheaney@chromium.org/13419002/9...

[commit-bot: I haz the power, 2013-05-21 19:30:41]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-21 19:59:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/matthewjheaney@chromium.org/13419002/1...

[commit-bot: I haz the power, 2013-05-21 20:23:07]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-21 22:37:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/matthewjheaney@chromium.org/13419002/1...

[commit-bot: I haz the power, 2013-05-21 23:05:55]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-22 00:53:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/matthewjheaney@chromium.org/13419002/1...

[commit-bot: I haz the power, 2013-05-22 08:08:12]

Retried try job too often on win7_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-05-22 18:51:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/matthewjheaney@chromium.org/13419002/1...

[commit-bot: I haz the power, 2013-05-23 06:47:55]

Change committed as 201716