Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23702007:
Render inband text tracks in the media pipeline (Closed)
|
Created:
7 years, 3 months ago by Matthew Heaney (Chromium) Modified:
7 years, 1 month ago CC:
chromium-reviews, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, jam, darin-cc_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionRender inband text tracks in the media pipeline
This change modifies the FFmpeg demuxer to recognize text
streams embedded in the source media (Webm). Text decoder
and text renderer filters have been added to the pipeline,
to process the text frames as they are pulled downstream.
BUG=230708
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=236660
Patch Set 1 #
Total comments: 106
Patch Set 2 : incorporate aaron's comments #Patch Set 3 : incorporated more of aaron's comments #
Total comments: 16
Patch Set 4 : incorporate aaron's comments (9/28) #
Total comments: 74
Patch Set 5 : incorporate aaron's comments (10/12) #
Total comments: 60
Patch Set 6 : incorporate aaron's comments (10/16) #
Total comments: 83
Patch Set 7 : incorporate aaron's comments (10/22) #
Total comments: 44
Patch Set 8 : incorporate aaron's comments (10/26)" #Patch Set 9 : modified ffmpeg demuxer unittest #
Total comments: 30
Patch Set 10 : incorporate aaron's comments (11/04) #
Total comments: 32
Patch Set 11 : incorporate aaron's comments (11/12) #
Total comments: 34
Patch Set 12 : incorporate aaron's comments 11/19 #Patch Set 13 : fix compile errors 11/20 #1 #Patch Set 14 : fix compile errors 11/20 #2 #Patch Set 15 : fix compile errors 11/20 #3 #
Total comments: 2
Patch Set 16 : fix compile errors 11/21 #4 #Patch Set 17 : fix compile errors 11/21 #5 #Patch Set 18 : fix compile errors 11/21 #6 #
Messages
Total messages: 54 (0 generated)
I just pushed up this CL for review. It adds support for inband text tracks to the ffmpeg-based pipepline. Note that the ffmpeg DEPS roll is stuck (see Chromium bug issue 281764), so neither the try-bots will work nor will you even be able to compile. Thanks, Matt
https://chromiumcodereview.appspot.com/23702007/diff/1/media/base/pipeline_un... File media/base/pipeline_unittest.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/1/media/base/pipeline_un... media/base/pipeline_unittest.cc:466: TEST_F(PipelineTest, VideoTextStream) { Maybe add a Text only stream test that will fail. https://chromiumcodereview.appspot.com/23702007/diff/1/media/base/text_buffer.cc File media/base/text_buffer.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/1/media/base/text_buffer... media/base/text_buffer.cc:21: if (buf == NULL || buflen <= 0) { Add DCHECK(str)
fyi: I'll try to get to reviewing this tomorrow. I've become a popular reviewer in the last few days and am still trying to dig out of my backlog. :)
Here is my initial round of comments. Some of them have cascading effects so I only made a comment in one place and assumed that the other necessary changes would be obvious. In general though, I like the direction this CL is headed. https://codereview.chromium.org/23702007/diff/1/content/renderer/media/webmed... File content/renderer/media/webmediaplayer_impl.cc (right): https://codereview.chromium.org/23702007/diff/1/content/renderer/media/webmed... content/renderer/media/webmediaplayer_impl.cc:972: if (ffmpeg_text_track_map_.find(index) == ffmpeg_text_track_map_.end()) { nit: It seems like we should DCHECK instead. It would be a bug if the index was already being used right? https://codereview.chromium.org/23702007/diff/1/content/renderer/media/webmed... content/renderer/media/webmediaplayer_impl.cc:1062: add_text_track_cb, This doesn't seem right to me. It seems like Demuxers should notify the pipeline to add/remove tracks through the DemuxerHost interface. This should in turn cause streams to be added/removed from the TextRenderer. It seems like the TextRenderer should be one to receive this particular callback. https://codereview.chromium.org/23702007/diff/1/content/renderer/media/webmed... content/renderer/media/webmediaplayer_impl.cc:1148: if (load_type_ != LoadTypeMediaSource && This should be load_type_ independent. ChunkDemuxer and FFmpegDemuxer should both use the text renderer mechanism. https://codereview.chromium.org/23702007/diff/1/content/renderer/media/webmed... content/renderer/media/webmediaplayer_impl.cc:1290: if (buf == NULL || buf->text().empty()) nit: These should be DCHECKS. https://codereview.chromium.org/23702007/diff/1/content/renderer/media/webmed... content/renderer/media/webmediaplayer_impl.cc:1293: TextTrackMap::iterator it = ffmpeg_text_track_map_.find(index); The TextRenderer seems like a better home for this map. https://codereview.chromium.org/23702007/diff/1/content/renderer/media/webmed... content/renderer/media/webmediaplayer_impl.cc:1294: if (it == ffmpeg_text_track_map_.end()) nit: Shouldn't this be a DCHECK? It seems like this would be a bug if we got a cue for a track we didn't have. https://codereview.chromium.org/23702007/diff/1/content/renderer/media/webmed... File content/renderer/media/webmediaplayer_impl.h (right): https://codereview.chromium.org/23702007/diff/1/content/renderer/media/webmed... content/renderer/media/webmediaplayer_impl.h:205: void OnFFmpegTextTrack(media::TextKind kind, A separate method should not be needed for FFmpegDemuxer vs ChunkDemuxer. This code should not be aware of the difference. https://codereview.chromium.org/23702007/diff/1/content/renderer/media/webmed... content/renderer/media/webmediaplayer_impl.h:383: TextTrackMap ffmpeg_text_track_map_; The ChunkDemuxer didn't need this map here. Why does the FFmpeg code need it? https://codereview.chromium.org/23702007/diff/1/media/base/demuxer.h File media/base/demuxer.h (right): https://codereview.chromium.org/23702007/diff/1/media/base/demuxer.h#newcode80 media/base/demuxer.h:80: virtual DemuxerStream* GetStreamByIndex(int index); I think this API can be problematic for the MSE case where tracks can come and go based on SourceBuffers coming and going. In general, I'd like to move away from the GetStream() style method and have the demuxer add/remove streams via the DemuxerHost with something along these lines. bool AddStream(DemuxerStream* stream); void RemoveStream(DemuxerStream* stream); This would remove the need for the index because the Pipeline could pass the DemuxerStream directly to the appropriate renderer. For now I think we should try this for TextTracks and if we like it, then I can migrate the audio & video code to use the same mechanism. https://codereview.chromium.org/23702007/diff/1/media/base/pipeline.h File media/base/pipeline.h (right): https://codereview.chromium.org/23702007/diff/1/media/base/pipeline.h#newcode143 media/base/pipeline.h:143: bool HasText() const; nit: This only appears to be used by test code. Do we really need it? https://codereview.chromium.org/23702007/diff/1/media/base/text_buffer.h File media/base/text_buffer.h (right): https://codereview.chromium.org/23702007/diff/1/media/base/text_buffer.h#newc... media/base/text_buffer.h:17: class MEDIA_EXPORT TextBuffer nit: s/TextBuffer/TextCue https://codereview.chromium.org/23702007/diff/1/media/base/text_buffer.h#newc... media/base/text_buffer.h:29: void set_id(const uint8* id, int id_size); nit: Any reason id, settings, and text can't be passed into the constructor? These should also be passed in as const std::string&. The conversion to std::string should happen in the format parsing code. https://codereview.chromium.org/23702007/diff/1/media/base/text_decoder.h File media/base/text_decoder.h (right): https://codereview.chromium.org/23702007/diff/1/media/base/text_decoder.h#new... media/base/text_decoder.h:23: virtual void Initialize(Demuxer* demuxer) = 0; ISTM that this should be taking a DemuxerStream and not a Demuxer. https://codereview.chromium.org/23702007/diff/1/media/base/text_decoder.h#new... media/base/text_decoder.h:37: virtual void Read(int index, const ReadCB& read_cb) = 0; Why do you need an index here? It seems like there should be a 1:1 relationship between TextDecoders & DemuxerStreams. https://codereview.chromium.org/23702007/diff/1/media/base/text_renderer.h File media/base/text_renderer.h (right): https://codereview.chromium.org/23702007/diff/1/media/base/text_renderer.h#ne... media/base/text_renderer.h:16: class MEDIA_EXPORT TextRenderer { I don't anticipate us having more than 1 TextRenderer impl so just get rid of this abstract interface and just make TextRendererImpl TextRenderer. https://codereview.chromium.org/23702007/diff/1/media/filters/chunk_demuxer.cc File media/filters/chunk_demuxer.cc (right): https://codereview.chromium.org/23702007/diff/1/media/filters/chunk_demuxer.c... media/filters/chunk_demuxer.cc:1346: case DemuxerStream::TEXT: ChunkDemuxer should be updated to expose text tracks the same way FFmpegDemuxer will. https://codereview.chromium.org/23702007/diff/1/media/filters/ffmpeg_demuxer.cc File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/23702007/diff/1/media/filters/ffmpeg_demuxer.... media/filters/ffmpeg_demuxer.cc:143: side_data.append(1, 0xFF); nit: Consider just using the null terminator and std::vector<uint8> instead. https://codereview.chromium.org/23702007/diff/1/media/filters/ffmpeg_demuxer.... media/filters/ffmpeg_demuxer.cc:150: uint8* side_data = av_packet_get_side_data( Why is this change needed for non-texttrack data? https://codereview.chromium.org/23702007/diff/1/media/filters/ffmpeg_demuxer.... media/filters/ffmpeg_demuxer.cc:165: if ((type() == DemuxerStream::AUDIO && audio_config_.is_encrypted()) || nit: I don't think you need to move this into the else block. https://codereview.chromium.org/23702007/diff/1/media/filters/ffmpeg_demuxer.... media/filters/ffmpeg_demuxer.cc:435: if (idx < 0 || StreamVector::size_type(idx) >= streams_.size()) { nit: These should be DCHECKs. https://codereview.chromium.org/23702007/diff/1/media/filters/ffmpeg_demuxer.... media/filters/ffmpeg_demuxer.cc:584: text_streams[i] = stream; nit: Any harm in doing the work below, right here? Perhaps in a helper function? https://codereview.chromium.org/23702007/diff/1/media/filters/ffmpeg_demuxer.... media/filters/ffmpeg_demuxer.cc:627: if (text_title != NULL && text_title->value != NULL) { nit: move av_dict_get() & NULL checks to a helper method so this code can look like std::string title = getMetadata(text_stream, "title"); std::string language = getMetadata(text_stream, "language"); I think that would make this a little easier to follow. https://codereview.chromium.org/23702007/diff/1/media/filters/ffmpeg_demuxer_... File media/filters/ffmpeg_demuxer_unittest.cc (right): https://codereview.chromium.org/23702007/diff/1/media/filters/ffmpeg_demuxer_... media/filters/ffmpeg_demuxer_unittest.cc:362: EXPECT_EQ(4, stream_count); nit: inline here since the count isn't used anywhere else. https://codereview.chromium.org/23702007/diff/1/media/filters/ffmpeg_text_dec... File media/filters/ffmpeg_text_decoder.cc (right): https://codereview.chromium.org/23702007/diff/1/media/filters/ffmpeg_text_dec... media/filters/ffmpeg_text_decoder.cc:33: // PipelineIntegrationTest.BasicPlayback. Do we really need this? I don't think we should be copying this code. It doesn't look like we are even using FFmpeg code in here. https://codereview.chromium.org/23702007/diff/1/media/filters/ffmpeg_text_dec... media/filters/ffmpeg_text_decoder.cc:71: void FFmpegTextDecoder::BufferReady( nit:There doesn't appear to be anything FFmpeg specific in this code so it should probably be renamed. https://codereview.chromium.org/23702007/diff/1/media/filters/text_renderer_i... File media/filters/text_renderer_impl.cc (right): https://codereview.chromium.org/23702007/diff/1/media/filters/text_renderer_i... media/filters/text_renderer_impl.cc:72: state_ = kStopped; It seems like state_ = kPaused and eos_count_ = text_stream_count_ should be set here instead. https://codereview.chromium.org/23702007/diff/1/media/filters/text_renderer_i... media/filters/text_renderer_impl.cc:74: base::ResetAndReturn(&init_cb_).Run(PIPELINE_OK); This method doesn't look like it actually needs to be asynchronous so there is no need for the init_cb parameter or init_cb_ member variable. https://codereview.chromium.org/23702007/diff/1/media/filters/text_renderer_i... media/filters/text_renderer_impl.cc:80: DCHECK_NE(state_, kUninitialized); nit: Remove since the DCHECK below is more specific and would fail if the condition here was true. https://codereview.chromium.org/23702007/diff/1/media/filters/text_renderer_i... media/filters/text_renderer_impl.cc:113: eos_count_ = text_stream_count_; Why is this line here? This doesn't feel right to me. https://codereview.chromium.org/23702007/diff/1/media/filters/text_renderer_i... media/filters/text_renderer_impl.cc:119: if (state_ == kPaused) { You shouldn't get a Pause() call if you are already paused. I think you should DCHECK above https://codereview.chromium.org/23702007/diff/1/media/filters/text_renderer_i... media/filters/text_renderer_impl.cc:125: if (state_ == kPausePending) You shouldn't get another Pause() call if the current one is still pending. Update DCHECKs https://codereview.chromium.org/23702007/diff/1/media/filters/text_renderer_i... media/filters/text_renderer_impl.cc:136: It seems like you would need a Flush() method so that you can properly set eos_count_ = text_stream_count_ on a seek. https://codereview.chromium.org/23702007/diff/1/media/filters/text_renderer_i... media/filters/text_renderer_impl.cc:145: if (state_ == kStopped) { You shound't get a stop if you have already received a Stop() call. https://codereview.chromium.org/23702007/diff/1/media/filters/text_renderer_i... media/filters/text_renderer_impl.cc:151: if (state_ == kStopPending) { You should not get a Stop() if the previous one is still pending. https://codereview.chromium.org/23702007/diff/1/media/filters/text_renderer_i... media/filters/text_renderer_impl.cc:170: DCHECK_NE(state_, kUninitialized); can't you DCHECK(message_loop_->BelongsToCurrentThread()) here? If not then you should BindToCurrentLoop() when you make the Read() calls so that you don't need the lock. https://codereview.chromium.org/23702007/diff/1/media/filters/text_renderer_i... media/filters/text_renderer_impl.cc:181: if (buffer.get()) nit: You shouldn't need the get() https://codereview.chromium.org/23702007/diff/1/media/filters/text_renderer_i... media/filters/text_renderer_impl.cc:188: if (pending_read_count_ > 0) DCHECK https://codereview.chromium.org/23702007/diff/1/media/filters/text_renderer_i... media/filters/text_renderer_impl.cc:192: state_ = kStopped; it seems like an kEnded state would be more appropriate to differentiate this state from the "stop has been called and we have run the stop callback." https://codereview.chromium.org/23702007/diff/1/media/filters/text_renderer_i... File media/filters/text_renderer_impl.h (right): https://codereview.chromium.org/23702007/diff/1/media/filters/text_renderer_i... media/filters/text_renderer_impl.h:44: virtual void Initialize(Demuxer* demuxer, I think there should be an AddStream(DemuxerStream) / RemoveStream(DemuxerStream) pair on this class that allows tracks to be added/removed during playback so we can properly handle the MSE case. This would remove the need for passing the demuxer to the Initialize() method. https://codereview.chromium.org/23702007/diff/1/media/filters/text_renderer_i... media/filters/text_renderer_impl.h:50: I think there should be an AddTrack() / RemoveTrack() pair on this class that allows tracks to be added/removed during playback so we can properly handle the MSE case. https://codereview.chromium.org/23702007/diff/1/media/filters/text_renderer_i... media/filters/text_renderer_impl.h:74: base::Lock lock_; Do you really need this lock? Doesn't this code only run on the media thread? https://codereview.chromium.org/23702007/diff/1/media/filters/text_renderer_i... media/filters/text_renderer_impl.h:101: // Indicates how many text streams have delivered end-of-stream indications. nit: This comment does not appear to match the code. IIUC it actually holds the number of streams that haven't ended yet.
One other thing to consider is to create a new DemuxerTextStream interface instead of DemuxerStream that returns TextBuffers instead of DecoderBuffers. That way the demuxer doesn't need to shoehorn the text data into a DecoderBuffer and then use a TextDecoder to "decode" it into TextBuffers.
Incorporated a slew of Aaron's comments. I did leave a few questions behind to confirm my understanding of the changes to DemuxerHost and friends. https://codereview.chromium.org/23702007/diff/1/content/renderer/media/webmed... File content/renderer/media/webmediaplayer_impl.cc (right): https://codereview.chromium.org/23702007/diff/1/content/renderer/media/webmed... content/renderer/media/webmediaplayer_impl.cc:972: if (ffmpeg_text_track_map_.find(index) == ffmpeg_text_track_map_.end()) { On 2013/09/12 00:15:15, acolwell wrote: > nit: It seems like we should DCHECK instead. It would be a bug if the index was > already being used right? Done. https://codereview.chromium.org/23702007/diff/1/content/renderer/media/webmed... content/renderer/media/webmediaplayer_impl.cc:1062: add_text_track_cb, On 2013/09/12 00:15:15, acolwell wrote: > This doesn't seem right to me. It seems like Demuxers should notify the pipeline > to add/remove tracks through the DemuxerHost interface. This should in turn > cause streams to be added/removed from the TextRenderer. It seems like the > TextRenderer should be one to receive this particular callback. OK, just to make sure I understand. The FFmpegDemuxer should hold a DemuxerHost pointer. When it detects a text stream, it should call AddStream on the DemuxerHost pointer, instead of calling the add_text_track_cb. Is my understanding correct? https://codereview.chromium.org/23702007/diff/1/content/renderer/media/webmed... content/renderer/media/webmediaplayer_impl.cc:1148: if (load_type_ != LoadTypeMediaSource && On 2013/09/12 00:15:15, acolwell wrote: > This should be load_type_ independent. ChunkDemuxer and FFmpegDemuxer should > both use the text renderer mechanism. Agreed. I figure I can make changes to DemuxerHost first, and then change the ChunkDemuxer. https://codereview.chromium.org/23702007/diff/1/content/renderer/media/webmed... content/renderer/media/webmediaplayer_impl.cc:1290: if (buf == NULL || buf->text().empty()) On 2013/09/12 00:15:15, acolwell wrote: > nit: These should be DCHECKS. Done. https://codereview.chromium.org/23702007/diff/1/content/renderer/media/webmed... content/renderer/media/webmediaplayer_impl.cc:1293: TextTrackMap::iterator it = ffmpeg_text_track_map_.find(index); On 2013/09/12 00:15:15, acolwell wrote: > The TextRenderer seems like a better home for this map. Agreed. I think this will go away once I make the changes to the DemuxerHost. https://codereview.chromium.org/23702007/diff/1/content/renderer/media/webmed... content/renderer/media/webmediaplayer_impl.cc:1294: if (it == ffmpeg_text_track_map_.end()) On 2013/09/12 00:15:15, acolwell wrote: > nit: Shouldn't this be a DCHECK? It seems like this would be a bug if we got a > cue for a track we didn't have. Done. https://codereview.chromium.org/23702007/diff/1/content/renderer/media/webmed... File content/renderer/media/webmediaplayer_impl.h (right): https://codereview.chromium.org/23702007/diff/1/content/renderer/media/webmed... content/renderer/media/webmediaplayer_impl.h:205: void OnFFmpegTextTrack(media::TextKind kind, On 2013/09/12 00:15:15, acolwell wrote: > A separate method should not be needed for FFmpegDemuxer vs ChunkDemuxer. This > code should not be aware of the difference. Agreed. I'm thinking I should make the DemuxerHost changes first, and then when that's working, change the ChunkDemuxer to use that too. https://codereview.chromium.org/23702007/diff/1/content/renderer/media/webmed... content/renderer/media/webmediaplayer_impl.h:383: TextTrackMap ffmpeg_text_track_map_; On 2013/09/12 00:15:15, acolwell wrote: > The ChunkDemuxer didn't need this map here. Why does the FFmpeg code need it? I was having some trouble getting the callback mechanism working (I'm still learning the threading model), and this was the solution that managed to get working. No surprise that it's not optimal. I think this will go away as a side effect of the DemuxerHost changes. https://codereview.chromium.org/23702007/diff/1/media/base/demuxer.h File media/base/demuxer.h (right): https://codereview.chromium.org/23702007/diff/1/media/base/demuxer.h#newcode80 media/base/demuxer.h:80: virtual DemuxerStream* GetStreamByIndex(int index); On 2013/09/12 00:15:15, acolwell wrote: > I think this API can be problematic for the MSE case where tracks can come and > go based on SourceBuffers coming and going. In general, I'd like to move away > from the GetStream() style method and have the demuxer add/remove streams via > the DemuxerHost with something along these lines. > > bool AddStream(DemuxerStream* stream); > void RemoveStream(DemuxerStream* stream); > > This would remove the need for the index because the Pipeline could pass the > DemuxerStream directly to the appropriate renderer. For now I think we should > try this for TextTracks and if we like it, then I can migrate the audio & video > code to use the same mechanism. It looks like the only class that inherits DemuxerHost is Pipeline. I think what you're suggesting here is that the demuxer should hold a pointer to the Pipeline (perhaps via the DemuxerHost interface), and that as it finds text streams, it should call its DemuxerHost with AddStream. The Pipeline, which implements AddStream, will forward that request to the text renderer, and the text renderer will adjust its container-of-streams accordingly. Is my understanding correct? https://codereview.chromium.org/23702007/diff/1/media/base/pipeline.h File media/base/pipeline.h (right): https://codereview.chromium.org/23702007/diff/1/media/base/pipeline.h#newcode143 media/base/pipeline.h:143: bool HasText() const; On 2013/09/12 00:15:15, acolwell wrote: > nit: This only appears to be used by test code. Do we really need it? I don't think so. We can replace it with some alternative, if you like. https://codereview.chromium.org/23702007/diff/1/media/base/text_buffer.cc File media/base/text_buffer.cc (right): https://codereview.chromium.org/23702007/diff/1/media/base/text_buffer.cc#new... media/base/text_buffer.cc:21: if (buf == NULL || buflen <= 0) { On 2013/09/04 22:23:09, fgalligan1 wrote: > Add DCHECK(str) OBE https://codereview.chromium.org/23702007/diff/1/media/base/text_buffer.h File media/base/text_buffer.h (right): https://codereview.chromium.org/23702007/diff/1/media/base/text_buffer.h#newc... media/base/text_buffer.h:17: class MEDIA_EXPORT TextBuffer On 2013/09/12 00:15:15, acolwell wrote: > nit: s/TextBuffer/TextCue Done. https://codereview.chromium.org/23702007/diff/1/media/base/text_buffer.h#newc... media/base/text_buffer.h:29: void set_id(const uint8* id, int id_size); On 2013/09/12 00:15:15, acolwell wrote: > nit: Any reason id, settings, and text can't be passed into the constructor? > These should also be passed in as const std::string&. The conversion to > std::string should happen in the format parsing code. Done. https://codereview.chromium.org/23702007/diff/1/media/base/text_decoder.h File media/base/text_decoder.h (right): https://codereview.chromium.org/23702007/diff/1/media/base/text_decoder.h#new... media/base/text_decoder.h:23: virtual void Initialize(Demuxer* demuxer) = 0; On 2013/09/12 00:15:15, acolwell wrote: > ISTM that this should be taking a DemuxerStream and not a Demuxer. The pipeline model is that there is one decoder per renderer. There is only a single text renderer (it's a state machine, basically), hence there is only a single text decoder. https://codereview.chromium.org/23702007/diff/1/media/base/text_decoder.h#new... media/base/text_decoder.h:37: virtual void Read(int index, const ReadCB& read_cb) = 0; On 2013/09/12 00:15:15, acolwell wrote: > Why do you need an index here? It seems like there should be a 1:1 relationship > between TextDecoders & DemuxerStreams. It doesn't work that way. There is only a single text decoder. It uses the index to associate read requests with a specific stream. https://codereview.chromium.org/23702007/diff/1/media/base/text_renderer.h File media/base/text_renderer.h (right): https://codereview.chromium.org/23702007/diff/1/media/base/text_renderer.h#ne... media/base/text_renderer.h:16: class MEDIA_EXPORT TextRenderer { On 2013/09/12 00:15:15, acolwell wrote: > I don't anticipate us having more than 1 TextRenderer impl so just get rid of > this abstract interface and just make TextRendererImpl TextRenderer. Done. https://codereview.chromium.org/23702007/diff/1/media/filters/chunk_demuxer.cc File media/filters/chunk_demuxer.cc (right): https://codereview.chromium.org/23702007/diff/1/media/filters/chunk_demuxer.c... media/filters/chunk_demuxer.cc:1346: case DemuxerStream::TEXT: On 2013/09/12 00:15:15, acolwell wrote: > ChunkDemuxer should be updated to expose text tracks the same way FFmpegDemuxer > will. OK, I can take care of that once the DemuxerHost changes have been made. https://codereview.chromium.org/23702007/diff/1/media/filters/ffmpeg_demuxer.cc File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/23702007/diff/1/media/filters/ffmpeg_demuxer.... media/filters/ffmpeg_demuxer.cc:143: side_data.append(1, 0xFF); On 2013/09/12 00:15:15, acolwell wrote: > nit: Consider just using the null terminator and std::vector<uint8> instead. I appended a NUL to each sub-string, instead of using 0xFF as a separator. https://codereview.chromium.org/23702007/diff/1/media/filters/ffmpeg_demuxer.... media/filters/ffmpeg_demuxer.cc:150: uint8* side_data = av_packet_get_side_data( On 2013/09/12 00:15:15, acolwell wrote: > Why is this change needed for non-texttrack data? I'm not quite following here. What change are you referring to? https://codereview.chromium.org/23702007/diff/1/media/filters/ffmpeg_demuxer.... media/filters/ffmpeg_demuxer.cc:165: if ((type() == DemuxerStream::AUDIO && audio_config_.is_encrypted()) || On 2013/09/12 00:15:15, acolwell wrote: > nit: I don't think you need to move this into the else block. Done. https://codereview.chromium.org/23702007/diff/1/media/filters/ffmpeg_demuxer.... media/filters/ffmpeg_demuxer.cc:435: if (idx < 0 || StreamVector::size_type(idx) >= streams_.size()) { On 2013/09/12 00:15:15, acolwell wrote: > nit: These should be DCHECKs. Done. https://codereview.chromium.org/23702007/diff/1/media/filters/ffmpeg_demuxer.... media/filters/ffmpeg_demuxer.cc:584: text_streams[i] = stream; On 2013/09/12 00:15:15, acolwell wrote: > nit: Any harm in doing the work below, right here? Perhaps in a helper function? We iterate through all the streams first, in order to determine whether we have at least one audio or video stream. The work we do after the iteration is to add a text track for each text steam. I was trying to eliminate any chance of a problem should we create text tracks, and then report DEMUXER_ERROR_NO_SUPPORTED_STREAMS because no audio or video was present. https://codereview.chromium.org/23702007/diff/1/media/filters/ffmpeg_demuxer.... media/filters/ffmpeg_demuxer.cc:627: if (text_title != NULL && text_title->value != NULL) { On 2013/09/12 00:15:15, acolwell wrote: > nit: move av_dict_get() & NULL checks to a helper method so this code can look > like > > std::string title = getMetadata(text_stream, "title"); > std::string language = getMetadata(text_stream, "language"); > > I think that would make this a little easier to follow. Done. https://codereview.chromium.org/23702007/diff/1/media/filters/ffmpeg_demuxer_... File media/filters/ffmpeg_demuxer_unittest.cc (right): https://codereview.chromium.org/23702007/diff/1/media/filters/ffmpeg_demuxer_... media/filters/ffmpeg_demuxer_unittest.cc:362: EXPECT_EQ(4, stream_count); On 2013/09/12 00:15:15, acolwell wrote: > nit: inline here since the count isn't used anywhere else. Done. https://codereview.chromium.org/23702007/diff/1/media/filters/ffmpeg_text_dec... File media/filters/ffmpeg_text_decoder.cc (right): https://codereview.chromium.org/23702007/diff/1/media/filters/ffmpeg_text_dec... media/filters/ffmpeg_text_decoder.cc:33: // PipelineIntegrationTest.BasicPlayback. On 2013/09/12 00:15:15, acolwell wrote: > Do we really need this? I don't think we should be copying this code. It doesn't > look like we are even using FFmpeg code in here. Done. https://codereview.chromium.org/23702007/diff/1/media/filters/ffmpeg_text_dec... media/filters/ffmpeg_text_decoder.cc:71: void FFmpegTextDecoder::BufferReady( On 2013/09/12 00:15:15, acolwell wrote: > nit:There doesn't appear to be anything FFmpeg specific in this code so it > should probably be renamed. Done. https://codereview.chromium.org/23702007/diff/1/media/filters/text_renderer_i... File media/filters/text_renderer_impl.cc (right): https://codereview.chromium.org/23702007/diff/1/media/filters/text_renderer_i... media/filters/text_renderer_impl.cc:72: state_ = kStopped; On 2013/09/12 00:15:15, acolwell wrote: > It seems like state_ = kPaused and eos_count_ = text_stream_count_ should be set > here instead. Done. https://codereview.chromium.org/23702007/diff/1/media/filters/text_renderer_i... media/filters/text_renderer_impl.cc:74: base::ResetAndReturn(&init_cb_).Run(PIPELINE_OK); On 2013/09/12 00:15:15, acolwell wrote: > This method doesn't look like it actually needs to be asynchronous so there is > no need for the init_cb parameter or init_cb_ member variable. Done. https://codereview.chromium.org/23702007/diff/1/media/filters/text_renderer_i... media/filters/text_renderer_impl.cc:80: DCHECK_NE(state_, kUninitialized); On 2013/09/12 00:15:15, acolwell wrote: > nit: Remove since the DCHECK below is more specific and would fail if the > condition here was true. Done. https://codereview.chromium.org/23702007/diff/1/media/filters/text_renderer_i... media/filters/text_renderer_impl.cc:113: eos_count_ = text_stream_count_; On 2013/09/12 00:15:15, acolwell wrote: > Why is this line here? This doesn't feel right to me. Done. https://codereview.chromium.org/23702007/diff/1/media/filters/text_renderer_i... media/filters/text_renderer_impl.cc:119: if (state_ == kPaused) { On 2013/09/12 00:15:15, acolwell wrote: > You shouldn't get a Pause() call if you are already paused. I think you should > DCHECK above Done. https://codereview.chromium.org/23702007/diff/1/media/filters/text_renderer_i... media/filters/text_renderer_impl.cc:125: if (state_ == kPausePending) On 2013/09/12 00:15:15, acolwell wrote: > You shouldn't get another Pause() call if the current one is still pending. > Update DCHECKs Done. https://codereview.chromium.org/23702007/diff/1/media/filters/text_renderer_i... media/filters/text_renderer_impl.cc:136: On 2013/09/12 00:15:15, acolwell wrote: > It seems like you would need a Flush() method so that you can properly set > eos_count_ = text_stream_count_ on a seek. Done. https://codereview.chromium.org/23702007/diff/1/media/filters/text_renderer_i... media/filters/text_renderer_impl.cc:145: if (state_ == kStopped) { On 2013/09/12 00:15:15, acolwell wrote: > You shound't get a stop if you have already received a Stop() call. Done. https://codereview.chromium.org/23702007/diff/1/media/filters/text_renderer_i... media/filters/text_renderer_impl.cc:151: if (state_ == kStopPending) { On 2013/09/12 00:15:15, acolwell wrote: > You should not get a Stop() if the previous one is still pending. Done. https://codereview.chromium.org/23702007/diff/1/media/filters/text_renderer_i... media/filters/text_renderer_impl.cc:170: DCHECK_NE(state_, kUninitialized); On 2013/09/12 00:15:15, acolwell wrote: > can't you DCHECK(message_loop_->BelongsToCurrentThread()) here? If not then you > should BindToCurrentLoop() when you make the Read() calls so that you don't need > the lock. Done. https://codereview.chromium.org/23702007/diff/1/media/filters/text_renderer_i... media/filters/text_renderer_impl.cc:181: if (buffer.get()) On 2013/09/12 00:15:15, acolwell wrote: > nit: You shouldn't need the get() Done. https://codereview.chromium.org/23702007/diff/1/media/filters/text_renderer_i... media/filters/text_renderer_impl.cc:188: if (pending_read_count_ > 0) On 2013/09/12 00:15:15, acolwell wrote: > DCHECK Multiple read requests can be pending simultaneously -- one for each text stream. https://codereview.chromium.org/23702007/diff/1/media/filters/text_renderer_i... media/filters/text_renderer_impl.cc:192: state_ = kStopped; On 2013/09/12 00:15:15, acolwell wrote: > it seems like an kEnded state would be more appropriate to differentiate this > state from the "stop has been called and we have run the stop callback." Done. https://codereview.chromium.org/23702007/diff/1/media/filters/text_renderer_i... File media/filters/text_renderer_impl.h (right): https://codereview.chromium.org/23702007/diff/1/media/filters/text_renderer_i... media/filters/text_renderer_impl.h:44: virtual void Initialize(Demuxer* demuxer, On 2013/09/12 00:15:15, acolwell wrote: > I think there should be an AddStream(DemuxerStream) / > RemoveStream(DemuxerStream) pair on this class that allows tracks to be > added/removed during playback so we can properly handle the MSE case. > > This would remove the need for passing the demuxer to the Initialize() method. OK. I had written you a few questions about the exact mechanism you had in mind, to confirm my understanding. I'm thinking that we'll have to make some other adjustments to the TextDecoder in order to handle this. https://codereview.chromium.org/23702007/diff/1/media/filters/text_renderer_i... media/filters/text_renderer_impl.h:50: On 2013/09/12 00:15:15, acolwell wrote: > I think there should be an AddTrack() / RemoveTrack() pair on this class that > allows tracks to be added/removed during playback so we can properly handle the > MSE case. OK, I'll look into that. https://codereview.chromium.org/23702007/diff/1/media/filters/text_renderer_i... media/filters/text_renderer_impl.h:74: base::Lock lock_; On 2013/09/12 00:15:15, acolwell wrote: > Do you really need this lock? Doesn't this code only run on the media thread? Done. https://codereview.chromium.org/23702007/diff/1/media/filters/text_renderer_i... media/filters/text_renderer_impl.h:101: // Indicates how many text streams have delivered end-of-stream indications. On 2013/09/12 00:15:15, acolwell wrote: > nit: This comment does not appear to match the code. IIUC it actually holds the > number of streams that haven't ended yet. Done.
Just answering questions. I'll hold of on further review until you have the DemuxerHost changes in place since I think that will cause a decent amount of churn. https://codereview.chromium.org/23702007/diff/1/content/renderer/media/webmed... File content/renderer/media/webmediaplayer_impl.cc (right): https://codereview.chromium.org/23702007/diff/1/content/renderer/media/webmed... content/renderer/media/webmediaplayer_impl.cc:1062: add_text_track_cb, On 2013/09/13 19:51:54, Matthew Heaney (Chromium) wrote: > On 2013/09/12 00:15:15, acolwell wrote: > > This doesn't seem right to me. It seems like Demuxers should notify the > pipeline > > to add/remove tracks through the DemuxerHost interface. This should in turn > > cause streams to be added/removed from the TextRenderer. It seems like the > > TextRenderer should be one to receive this particular callback. > > OK, just to make sure I understand. The FFmpegDemuxer should hold a DemuxerHost > pointer. When it detects a text stream, it should call AddStream on the > DemuxerHost pointer, instead of calling the add_text_track_cb. > > Is my understanding correct? Yes. The demuxers already hold a reference to the DemuxerHost. It is passed in via the Initialize() method. https://codereview.chromium.org/23702007/diff/1/media/base/demuxer.h File media/base/demuxer.h (right): https://codereview.chromium.org/23702007/diff/1/media/base/demuxer.h#newcode80 media/base/demuxer.h:80: virtual DemuxerStream* GetStreamByIndex(int index); On 2013/09/13 19:51:54, Matthew Heaney (Chromium) wrote: > On 2013/09/12 00:15:15, acolwell wrote: > > I think this API can be problematic for the MSE case where tracks can come and > > go based on SourceBuffers coming and going. In general, I'd like to move away > > from the GetStream() style method and have the demuxer add/remove streams via > > the DemuxerHost with something along these lines. > > > > bool AddStream(DemuxerStream* stream); > > void RemoveStream(DemuxerStream* stream); > > > > This would remove the need for the index because the Pipeline could pass the > > DemuxerStream directly to the appropriate renderer. For now I think we should > > try this for TextTracks and if we like it, then I can migrate the audio & > video > > code to use the same mechanism. > > > It looks like the only class that inherits DemuxerHost is Pipeline. I think > what you're suggesting here is that the demuxer should hold a pointer to the > Pipeline (perhaps via the DemuxerHost interface), and that as it finds text > streams, it should call its DemuxerHost with AddStream. > Yes. At this point I think signatures along the lines of bool AddTextStream(DemuxerTextStream*); void RemoveTextStream(DemuxerTextStream*); should work. I am suggesting DemuxerTextStream instead of DemuxerStream because then you could avoid having to cram the TextCue data into a DecoderBuffer. You could just have the demuxer return the data in a more natural form and not have to do the delimited side-data like you have now. > The Pipeline, which implements AddStream, will forward that request to the text > renderer, and the text renderer will adjust its container-of-streams > accordingly. Exactly. > > Is my understanding correct? Yes. https://codereview.chromium.org/23702007/diff/1/media/base/text_decoder.h File media/base/text_decoder.h (right): https://codereview.chromium.org/23702007/diff/1/media/base/text_decoder.h#new... media/base/text_decoder.h:23: virtual void Initialize(Demuxer* demuxer) = 0; On 2013/09/13 19:51:54, Matthew Heaney (Chromium) wrote: > On 2013/09/12 00:15:15, acolwell wrote: > > ISTM that this should be taking a DemuxerStream and not a Demuxer. > > The pipeline model is that there is one decoder per renderer. There is only a > single text renderer (it's a state machine, basically), hence there is only a > single text decoder. Text is blazing a new trail here since it is the first datatype to actually support multiple tracks. When we actually implement multiple audio & video tracks, I believe we will have a model where 1 renderer will have multiple decoders. This is because those renderers may consume multiple tracks, but it is responsible for combining them into a single output. In the case of text tracks, it isn't clear to me that you'll actually need a TextDecoder once you have a DemuxerTextStream that actually hands out parsed queues. It seems like the renderer will just be responsible for pulling cues from the DemuxerTextStream and forwarding them to the media::TextTrack that is created by WebMediaPlayerImpl. It is also here in the TextRenderer that I think you'll be able to handle your "dropped cue" problem in the other CL. I think the renderer shouldn't hand out cues until play is called on the renderer. That way you should be able to enable the text track before playback begins. That is just my current hunch based on how this CL is shaping up. I'm fine if that fix is deferred to a followup CL. https://codereview.chromium.org/23702007/diff/1/media/base/text_decoder.h#new... media/base/text_decoder.h:37: virtual void Read(int index, const ReadCB& read_cb) = 0; On 2013/09/13 19:51:54, Matthew Heaney (Chromium) wrote: > On 2013/09/12 00:15:15, acolwell wrote: > > Why do you need an index here? It seems like there should be a 1:1 > relationship > > between TextDecoders & DemuxerStreams. > > It doesn't work that way. There is only a single text decoder. It uses the > index to associate read requests with a specific stream. Right. I was trying to say that I think there should be 1 decoder instance per stream since that is likely what would be the case for audio & video when multi-track support is added to those track types. Since I don't think you'll need this decoder class anymore, this is likely moot. https://codereview.chromium.org/23702007/diff/1/media/filters/ffmpeg_demuxer.cc File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/23702007/diff/1/media/filters/ffmpeg_demuxer.... media/filters/ffmpeg_demuxer.cc:150: uint8* side_data = av_packet_get_side_data( On 2013/09/13 19:51:54, Matthew Heaney (Chromium) wrote: > On 2013/09/12 00:15:15, acolwell wrote: > > Why is this change needed for non-texttrack data? > > I'm not quite following here. What change are you referring to? IIUC This is the non-text track code path. The original code does not appear to deal with side_data, so I was wondering why you added it here. It just seems like a change in behavior that is unrelated to the text track work. https://codereview.chromium.org/23702007/diff/1/media/filters/ffmpeg_demuxer.... media/filters/ffmpeg_demuxer.cc:584: text_streams[i] = stream; On 2013/09/13 19:51:54, Matthew Heaney (Chromium) wrote: > On 2013/09/12 00:15:15, acolwell wrote: > > nit: Any harm in doing the work below, right here? Perhaps in a helper > function? > > We iterate through all the streams first, in order to determine whether we have > at least one audio or video stream. > > The work we do after the iteration is to add a text track for each text steam. > I was trying to eliminate any chance of a problem should we create text tracks, > and then report DEMUXER_ERROR_NO_SUPPORTED_STREAMS because no audio or video was > present. I don't think this should be a problem. Reads on the streams should occur until the demuxer has successfully initialized so I think you should be ok. We can always add DCHECKS to enforce this. https://codereview.chromium.org/23702007/diff/1/media/filters/text_renderer_i... File media/filters/text_renderer_impl.cc (right): https://codereview.chromium.org/23702007/diff/1/media/filters/text_renderer_i... media/filters/text_renderer_impl.cc:188: if (pending_read_count_ > 0) On 2013/09/13 19:51:54, Matthew Heaney (Chromium) wrote: > On 2013/09/12 00:15:15, acolwell wrote: > > DCHECK > > Multiple read requests can be pending simultaneously -- one for each text > stream. Ok. Right. I don't know what I was thinking here.
I incorporated (most of) Aaron's suggestions. One issue that came up is that TextTrack manipulation must be done on the main thread (not the media thread), so that influenced a few design decisions. https://codereview.chromium.org/23702007/diff/1/content/renderer/media/webmed... File content/renderer/media/webmediaplayer_impl.cc (right): https://codereview.chromium.org/23702007/diff/1/content/renderer/media/webmed... content/renderer/media/webmediaplayer_impl.cc:1062: add_text_track_cb, On 2013/09/13 20:57:30, acolwell wrote: > On 2013/09/13 19:51:54, Matthew Heaney (Chromium) wrote: > > On 2013/09/12 00:15:15, acolwell wrote: > > > This doesn't seem right to me. It seems like Demuxers should notify the > > pipeline > > > to add/remove tracks through the DemuxerHost interface. This should in turn > > > cause streams to be added/removed from the TextRenderer. It seems like the > > > TextRenderer should be one to receive this particular callback. > > > > OK, just to make sure I understand. The FFmpegDemuxer should hold a > DemuxerHost > > pointer. When it detects a text stream, it should call AddStream on the > > DemuxerHost pointer, instead of calling the add_text_track_cb. > > > > Is my understanding correct? > Yes. The demuxers already hold a reference to the DemuxerHost. It is passed in > via the Initialize() method. > The ffmpeg demuxer now forwards the AddTextStream request to the DemuxerHost, and the Pipeline in turn forwards it to the TextRenderer. https://codereview.chromium.org/23702007/diff/1/content/renderer/media/webmed... content/renderer/media/webmediaplayer_impl.cc:1293: TextTrackMap::iterator it = ffmpeg_text_track_map_.find(index); On 2013/09/13 19:51:54, Matthew Heaney (Chromium) wrote: > On 2013/09/12 00:15:15, acolwell wrote: > > The TextRenderer seems like a better home for this map. > > Agreed. I think this will go away once I make the changes to the DemuxerHost. Actually, I had to leave this here, so that it could be run on the main thread. This is required by Blink. https://codereview.chromium.org/23702007/diff/1/content/renderer/media/webmed... File content/renderer/media/webmediaplayer_impl.h (right): https://codereview.chromium.org/23702007/diff/1/content/renderer/media/webmed... content/renderer/media/webmediaplayer_impl.h:205: void OnFFmpegTextTrack(media::TextKind kind, On 2013/09/13 19:51:54, Matthew Heaney (Chromium) wrote: > On 2013/09/12 00:15:15, acolwell wrote: > > A separate method should not be needed for FFmpegDemuxer vs ChunkDemuxer. This > > code should not be aware of the difference. > > Agreed. I'm thinking I should make the DemuxerHost changes first, and then when > that's working, change the ChunkDemuxer to use that too. I haven't touched the ChunkDemuxer yet. I just wanted to make sure I had the threading model correct first. Assuming it's correct, I can either fix the ChunkDemuxer in a separate patch for this CL, or defer the change to a future CL. https://codereview.chromium.org/23702007/diff/1/content/renderer/media/webmed... content/renderer/media/webmediaplayer_impl.h:383: TextTrackMap ffmpeg_text_track_map_; On 2013/09/13 19:51:54, Matthew Heaney (Chromium) wrote: > On 2013/09/12 00:15:15, acolwell wrote: > > The ChunkDemuxer didn't need this map here. Why does the FFmpeg code need it? > > I was having some trouble getting the callback mechanism working (I'm still > learning the threading model), and this was the solution that managed to get > working. No surprise that it's not optimal. I think this will go away as a > side effect of the DemuxerHost changes. As it turns out, creating a TextTrack(Impl) object must be done on the main thread (there's a check in WebMediaPlayerImpl::client()). Text cues must also be dispatched to the Blink on the main thread (there's a check deep inside blink). So I made the decision to just keep all knowledge of TextTrack(Impl) objects in WebMediaPlayerImpl, so I left this map here. (Though in this patch, the key type did change from int to DemuxerStream.) https://codereview.chromium.org/23702007/diff/1/media/base/demuxer.h File media/base/demuxer.h (right): https://codereview.chromium.org/23702007/diff/1/media/base/demuxer.h#newcode80 media/base/demuxer.h:80: virtual DemuxerStream* GetStreamByIndex(int index); On 2013/09/13 20:57:30, acolwell wrote: > On 2013/09/13 19:51:54, Matthew Heaney (Chromium) wrote: > > On 2013/09/12 00:15:15, acolwell wrote: > > > I think this API can be problematic for the MSE case where tracks can come > and > > > go based on SourceBuffers coming and going. In general, I'd like to move > away > > > from the GetStream() style method and have the demuxer add/remove streams > via > > > the DemuxerHost with something along these lines. > > > > > > bool AddStream(DemuxerStream* stream); > > > void RemoveStream(DemuxerStream* stream); > > > > > > This would remove the need for the index because the Pipeline could pass the > > > DemuxerStream directly to the appropriate renderer. For now I think we > should > > > try this for TextTracks and if we like it, then I can migrate the audio & > > video > > > code to use the same mechanism. > > > > > > It looks like the only class that inherits DemuxerHost is Pipeline. I think > > what you're suggesting here is that the demuxer should hold a pointer to the > > Pipeline (perhaps via the DemuxerHost interface), and that as it finds text > > streams, it should call its DemuxerHost with AddStream. > > > > Yes. At this point I think signatures along the lines of > > bool AddTextStream(DemuxerTextStream*); > void RemoveTextStream(DemuxerTextStream*); > > should work. I am suggesting DemuxerTextStream instead of DemuxerStream because > then you could avoid having to cram the TextCue data into a DecoderBuffer. You > could just have the demuxer return the data in a more natural form and not have > to do the delimited side-data like you have now. > > > The Pipeline, which implements AddStream, will forward that request to the > text > > renderer, and the text renderer will adjust its container-of-streams > > accordingly. > > Exactly. > > > > > Is my understanding correct? > > Yes. > I made this change, but haven't introduced a separate DemuxerTextStream type just yet. Such a type would complicate the implementation of the demuxer. (Not that that's a bad thing, I just wanted to get all the big issues such as the threading model working first.) https://codereview.chromium.org/23702007/diff/1/media/base/pipeline.h File media/base/pipeline.h (right): https://codereview.chromium.org/23702007/diff/1/media/base/pipeline.h#newcode143 media/base/pipeline.h:143: bool HasText() const; On 2013/09/13 19:51:54, Matthew Heaney (Chromium) wrote: > On 2013/09/12 00:15:15, acolwell wrote: > > nit: This only appears to be used by test code. Do we really need it? > > I don't think so. We can replace it with some alternative, if you like. I finally removed it. https://codereview.chromium.org/23702007/diff/1/media/base/text_decoder.h File media/base/text_decoder.h (right): https://codereview.chromium.org/23702007/diff/1/media/base/text_decoder.h#new... media/base/text_decoder.h:23: virtual void Initialize(Demuxer* demuxer) = 0; On 2013/09/13 20:57:30, acolwell wrote: > On 2013/09/13 19:51:54, Matthew Heaney (Chromium) wrote: > > On 2013/09/12 00:15:15, acolwell wrote: > > > ISTM that this should be taking a DemuxerStream and not a Demuxer. > > > > The pipeline model is that there is one decoder per renderer. There is only a > > single text renderer (it's a state machine, basically), hence there is only a > > single text decoder. > > Text is blazing a new trail here since it is the first datatype to actually > support multiple tracks. When we actually implement multiple audio & video > tracks, I believe we will have a model where 1 renderer will have multiple > decoders. This is because those renderers may consume multiple tracks, but it is > responsible for combining them into a single output. > > In the case of text tracks, it isn't clear to me that you'll actually need a > TextDecoder once you have a DemuxerTextStream that actually hands out parsed > queues. It seems like the renderer will just be responsible for pulling cues > from the DemuxerTextStream and forwarding them to the media::TextTrack that is > created by WebMediaPlayerImpl. > > It is also here in the TextRenderer that I think you'll be able to handle your > "dropped cue" problem in the other CL. I think the renderer shouldn't hand out > cues until play is called on the renderer. That way you should be able to enable > the text track before playback begins. That is just my current hunch based on > how this CL is shaping up. I'm fine if that fix is deferred to a followup CL. I haven't done anything yet with a DemuxerTextStream, so this patch set still contains a separate TextDecoder. I wanted to get the big issues worked out first. https://codereview.chromium.org/23702007/diff/1/media/base/text_decoder.h#new... media/base/text_decoder.h:37: virtual void Read(int index, const ReadCB& read_cb) = 0; On 2013/09/13 20:57:30, acolwell wrote: > On 2013/09/13 19:51:54, Matthew Heaney (Chromium) wrote: > > On 2013/09/12 00:15:15, acolwell wrote: > > > Why do you need an index here? It seems like there should be a 1:1 > > relationship > > > between TextDecoders & DemuxerStreams. > > > > It doesn't work that way. There is only a single text decoder. It uses the > > index to associate read requests with a specific stream. > > Right. I was trying to say that I think there should be 1 decoder instance per > stream since that is likely what would be the case for audio & video when > multi-track support is added to those track types. Since I don't think you'll > need this decoder class anymore, this is likely moot. I did change this from int to DemuxerStream, but the TextDecoder is still there. You're right that the TextDecoder doesn't do much, and it might be possible to get rid of it. I just needed to make sure the threading issues got resolved first. https://codereview.chromium.org/23702007/diff/1/media/filters/ffmpeg_demuxer.cc File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/23702007/diff/1/media/filters/ffmpeg_demuxer.... media/filters/ffmpeg_demuxer.cc:150: uint8* side_data = av_packet_get_side_data( On 2013/09/13 20:57:30, acolwell wrote: > On 2013/09/13 19:51:54, Matthew Heaney (Chromium) wrote: > > On 2013/09/12 00:15:15, acolwell wrote: > > > Why is this change needed for non-texttrack data? > > > > I'm not quite following here. What change are you referring to? > > IIUC This is the non-text track code path. The original code does not appear to > deal with side_data, so I was wondering why you added it here. It just seems > like a change in behavior that is unrelated to the text track work. I checked master again -- this bit of code was already there. It only applied to A/V streams so I moved it to the else part of the predicate. https://codereview.chromium.org/23702007/diff/1/media/filters/ffmpeg_demuxer.... media/filters/ffmpeg_demuxer.cc:584: text_streams[i] = stream; On 2013/09/13 20:57:30, acolwell wrote: > On 2013/09/13 19:51:54, Matthew Heaney (Chromium) wrote: > > On 2013/09/12 00:15:15, acolwell wrote: > > > nit: Any harm in doing the work below, right here? Perhaps in a helper > > function? > > > > We iterate through all the streams first, in order to determine whether we > have > > at least one audio or video stream. > > > > The work we do after the iteration is to add a text track for each text steam. > > > I was trying to eliminate any chance of a problem should we create text > tracks, > > and then report DEMUXER_ERROR_NO_SUPPORTED_STREAMS because no audio or video > was > > present. > > I don't think this should be a problem. Reads on the streams should occur until > the demuxer has successfully initialized so I think you should be ok. We can > always add DCHECKS to enforce this. I ended up moving that loop into its own subprogram. I couldn't notify the DemuxerHost of the text streams until actual DemuxerStream objects had been created. https://codereview.chromium.org/23702007/diff/1/media/filters/ffmpeg_demuxer_... File media/filters/ffmpeg_demuxer_unittest.cc (right): https://codereview.chromium.org/23702007/diff/1/media/filters/ffmpeg_demuxer_... media/filters/ffmpeg_demuxer_unittest.cc:362: EXPECT_EQ(4, stream_count); On 2013/09/13 19:51:54, Matthew Heaney (Chromium) wrote: > On 2013/09/12 00:15:15, acolwell wrote: > > nit: inline here since the count isn't used anywhere else. > > Done. This is OBE anyway, since GetStreamCount and friends is now gone. https://codereview.chromium.org/23702007/diff/1/media/filters/text_renderer_i... File media/filters/text_renderer_impl.h (right): https://codereview.chromium.org/23702007/diff/1/media/filters/text_renderer_i... media/filters/text_renderer_impl.h:44: virtual void Initialize(Demuxer* demuxer, On 2013/09/13 19:51:54, Matthew Heaney (Chromium) wrote: > On 2013/09/12 00:15:15, acolwell wrote: > > I think there should be an AddStream(DemuxerStream) / > > RemoveStream(DemuxerStream) pair on this class that allows tracks to be > > added/removed during playback so we can properly handle the MSE case. > > > > This would remove the need for passing the demuxer to the Initialize() method. > > > OK. I had written you a few questions about the exact mechanism you had in > mind, to confirm my understanding. > > I'm thinking that we'll have to make some other adjustments to the TextDecoder > in order to handle this. I added the new method(s) to DemuxerHost and Pipeline.
I didn't do a full review since it appears that you just wanted answers about
the threading issues first. I think the key points are to hide the thread
hopping in TextTrackImpl and TextRenderer should own the map of TextTrack
objects.
1. TextTrackImpl::addWebVTTCue() should PostTask() to the renderer thread and
then make the appropriate call on the client.
2. ~TextTrackImpl() should transfer ownership of text_track_ to a callback
posted to the renderer thread. That task should then call remove task on the
client and destroy the text_track. It should look something like this:
static void OnRemoveTrack(scoped_ptr<WebInbandTextTrackImpl> text_track) {
}
TextTrackImpl::~TextTrackImpl()
message_loop_proxy_->PostTask(
}
https://codereview.chromium.org/23702007/diff/24001/content/renderer/media/we...
File content/renderer/media/webmediaplayer_impl.cc (right):
https://codereview.chromium.org/23702007/diff/24001/content/renderer/media/we...
content/renderer/media/webmediaplayer_impl.cc:973: text_track_map_[text_stream]
= text_track.release();
I don't think you need the map here. You can hide the thread hopping business in
TextTrackImpl and let the renderer own the TextTrack instance.
I think if you use the following signature instead you'd avoid the need for the
map here and avoid map lookups on each cue. It would also allow the TextRenderer
to control the text track lifetime instead of having to coordinate with this map
here.
typedef base::Callback<void(scoped_ptr<media::TextTrack>)>
AddTextTrack(media::TextKind kind,
const std::string& label,
const std::string& language,
const AddTextTrackDoneCB& done_cb)
https://codereview.chromium.org/23702007/diff/24001/content/renderer/media/we...
content/renderer/media/webmediaplayer_impl.cc:1281: void
WebMediaPlayerImpl::CueReady(
You shouldn't need this. You can hide the thread hopping in TextTrackImpl and
the TextRenderer should just be able to interact with TextTrack objects.
https://codereview.chromium.org/23702007/diff/24001/media/base/demuxer.h
File media/base/demuxer.h (right):
https://codereview.chromium.org/23702007/diff/24001/media/base/demuxer.h#newc...
media/base/demuxer.h:15: #include "media/base/text_track.h"
nit: This doesn't appear to be needed.
https://codereview.chromium.org/23702007/diff/24001/media/base/demuxer.h#newc...
media/base/demuxer.h:19: class DemuxerStream;
nit: This shouldn't be needed since demuxer_stream.h is being included above.
https://codereview.chromium.org/23702007/diff/24001/media/base/mock_filters.h
File media/base/mock_filters.h (right):
https://codereview.chromium.org/23702007/diff/24001/media/base/mock_filters.h...
media/base/mock_filters.h:107: class MockTextDecoder : public TextDecoder {
nit: This class doesn't appear to be used anywhere. Please remove.
https://codereview.chromium.org/23702007/diff/24001/media/base/pipeline.cc
File media/base/pipeline.cc (right):
https://codereview.chromium.org/23702007/diff/24001/media/base/pipeline.cc#ne...
media/base/pipeline.cc:24: #include "media/base/text_decoder.h"
nit: It doesn't look like this is needed anywhere. Please remove.
https://codereview.chromium.org/23702007/diff/24001/media/base/pipeline.cc#ne...
media/base/pipeline.cc:913: if (text_renderer_ && !text_ended_)
I think you may need to add a HasTracks() method on text_renderer_ otherwise I
don't think playback will end for content w/o text tracks.
https://codereview.chromium.org/23702007/diff/24001/media/base/text_cue.h
File media/base/text_cue.h (right):
https://codereview.chromium.org/23702007/diff/24001/media/base/text_cue.h#new...
media/base/text_cue.h:43: DISALLOW_COPY_AND_ASSIGN(TextCue);
nit: s/DISALLOW_COPY_AND_ASSIGN/DISALLOW_IMPLICIT_CONSTRUCTORS
I didn't do a full review since it appears that you just wanted answers about
the threading issues first. I think the key points are to hide the thread
hopping in TextTrackImpl and TextRenderer should own the map of TextTrack
objects.
1. TextTrackImpl::addWebVTTCue() should PostTask() to the renderer thread and
then make the appropriate call on the client.
2. ~TextTrackImpl() should transfer ownership of text_track_ to a callback
posted to the renderer thread. That task should then call remove task on the
client and destroy the text_track. It should look something like this:
static void OnRemoveTrack(scoped_ptr<WebInbandTextTrackImpl> text_track) {
}
TextTrackImpl::~TextTrackImpl()
message_loop_proxy_->PostTask(
}
https://codereview.chromium.org/23702007/diff/24001/content/renderer/media/we...
File content/renderer/media/webmediaplayer_impl.cc (right):
https://codereview.chromium.org/23702007/diff/24001/content/renderer/media/we...
content/renderer/media/webmediaplayer_impl.cc:973: text_track_map_[text_stream]
= text_track.release();
I don't think you need the map here. You can hide the thread hopping business in
TextTrackImpl and let the renderer own the TextTrack instance.
I think if you use the following signature instead you'd avoid the need for the
map here and avoid map lookups on each cue. It would also allow the TextRenderer
to control the text track lifetime instead of having to coordinate with this map
here.
typedef base::Callback<void(scoped_ptr<media::TextTrack>)>
AddTextTrack(media::TextKind kind,
const std::string& label,
const std::string& language,
const AddTextTrackDoneCB& done_cb)
https://codereview.chromium.org/23702007/diff/24001/content/renderer/media/we...
content/renderer/media/webmediaplayer_impl.cc:1281: void
WebMediaPlayerImpl::CueReady(
You shouldn't need this. You can hide the thread hopping in TextTrackImpl and
the TextRenderer should just be able to interact with TextTrack objects.
https://codereview.chromium.org/23702007/diff/24001/media/base/demuxer.h
File media/base/demuxer.h (right):
https://codereview.chromium.org/23702007/diff/24001/media/base/demuxer.h#newc...
media/base/demuxer.h:15: #include "media/base/text_track.h"
nit: This doesn't appear to be needed.
https://codereview.chromium.org/23702007/diff/24001/media/base/demuxer.h#newc...
media/base/demuxer.h:19: class DemuxerStream;
nit: This shouldn't be needed since demuxer_stream.h is being included above.
https://codereview.chromium.org/23702007/diff/24001/media/base/mock_filters.h
File media/base/mock_filters.h (right):
https://codereview.chromium.org/23702007/diff/24001/media/base/mock_filters.h...
media/base/mock_filters.h:107: class MockTextDecoder : public TextDecoder {
nit: This class doesn't appear to be used anywhere. Please remove.
https://codereview.chromium.org/23702007/diff/24001/media/base/pipeline.cc
File media/base/pipeline.cc (right):
https://codereview.chromium.org/23702007/diff/24001/media/base/pipeline.cc#ne...
media/base/pipeline.cc:24: #include "media/base/text_decoder.h"
nit: It doesn't look like this is needed anywhere. Please remove.
https://codereview.chromium.org/23702007/diff/24001/media/base/pipeline.cc#ne...
media/base/pipeline.cc:913: if (text_renderer_ && !text_ended_)
I think you may need to add a HasTracks() method on text_renderer_ otherwise I
don't think playback will end for content w/o text tracks.
https://codereview.chromium.org/23702007/diff/24001/media/base/text_cue.h
File media/base/text_cue.h (right):
https://codereview.chromium.org/23702007/diff/24001/media/base/text_cue.h#new...
media/base/text_cue.h:43: DISALLOW_COPY_AND_ASSIGN(TextCue);
nit: s/DISALLOW_COPY_AND_ASSIGN/DISALLOW_IMPLICIT_CONSTRUCTORS
I didn't do a full review since it appears that you just wanted answers about
the threading issues first. I think the key points are to hide the thread
hopping in TextTrackImpl and TextRenderer should own the map of TextTrack
objects.
1. TextTrackImpl::addWebVTTCue() should PostTask() to the renderer thread and
then make the appropriate call on the client.
2. ~TextTrackImpl() should transfer ownership of text_track_ to a callback
posted to the renderer thread. That task should then call remove task on the
client and destroy the text_track. It should look something like this:
static void OnRemoveTrack(scoped_ptr<WebInbandTextTrackImpl> text_track) {
}
TextTrackImpl::~TextTrackImpl()
message_loop_proxy_->PostTask(
}
https://codereview.chromium.org/23702007/diff/24001/content/renderer/media/we...
File content/renderer/media/webmediaplayer_impl.cc (right):
https://codereview.chromium.org/23702007/diff/24001/content/renderer/media/we...
content/renderer/media/webmediaplayer_impl.cc:973: text_track_map_[text_stream]
= text_track.release();
I don't think you need the map here. You can hide the thread hopping business in
TextTrackImpl and let the renderer own the TextTrack instance.
I think if you use the following signature instead you'd avoid the need for the
map here and avoid map lookups on each cue. It would also allow the TextRenderer
to control the text track lifetime instead of having to coordinate with this map
here.
typedef base::Callback<void(scoped_ptr<media::TextTrack>)>
AddTextTrack(media::TextKind kind,
const std::string& label,
const std::string& language,
const AddTextTrackDoneCB& done_cb)
https://codereview.chromium.org/23702007/diff/24001/content/renderer/media/we...
content/renderer/media/webmediaplayer_impl.cc:1281: void
WebMediaPlayerImpl::CueReady(
You shouldn't need this. You can hide the thread hopping in TextTrackImpl and
the TextRenderer should just be able to interact with TextTrack objects.
https://codereview.chromium.org/23702007/diff/24001/media/base/demuxer.h
File media/base/demuxer.h (right):
https://codereview.chromium.org/23702007/diff/24001/media/base/demuxer.h#newc...
media/base/demuxer.h:15: #include "media/base/text_track.h"
nit: This doesn't appear to be needed.
https://codereview.chromium.org/23702007/diff/24001/media/base/demuxer.h#newc...
media/base/demuxer.h:19: class DemuxerStream;
nit: This shouldn't be needed since demuxer_stream.h is being included above.
https://codereview.chromium.org/23702007/diff/24001/media/base/mock_filters.h
File media/base/mock_filters.h (right):
https://codereview.chromium.org/23702007/diff/24001/media/base/mock_filters.h...
media/base/mock_filters.h:107: class MockTextDecoder : public TextDecoder {
nit: This class doesn't appear to be used anywhere. Please remove.
https://codereview.chromium.org/23702007/diff/24001/media/base/pipeline.cc
File media/base/pipeline.cc (right):
https://codereview.chromium.org/23702007/diff/24001/media/base/pipeline.cc#ne...
media/base/pipeline.cc:24: #include "media/base/text_decoder.h"
nit: It doesn't look like this is needed anywhere. Please remove.
https://codereview.chromium.org/23702007/diff/24001/media/base/pipeline.cc#ne...
media/base/pipeline.cc:913: if (text_renderer_ && !text_ended_)
I think you may need to add a HasTracks() method on text_renderer_ otherwise I
don't think playback will end for content w/o text tracks.
https://codereview.chromium.org/23702007/diff/24001/media/base/text_cue.h
File media/base/text_cue.h (right):
https://codereview.chromium.org/23702007/diff/24001/media/base/text_cue.h#new...
media/base/text_cue.h:43: DISALLOW_COPY_AND_ASSIGN(TextCue);
nit: s/DISALLOW_COPY_AND_ASSIGN/DISALLOW_IMPLICIT_CONSTRUCTORS
Accidentally sent too early.
static void OnRemoveTrack(scoped_ptr<WebInbandTextTrackImpl> text_track) {
if (text_track->client())
client_->removeTextTrack(text_track.get());
}
TextTrackImpl::~TextTrackImpl()
message_loop_proxy_->PostTask(FROM_HERE, base::Bind(&OnRemoveTrack,
text_track_.Pass()));
}
This should allow the track to get removed on the correct thread and simplify
ownership issues.
Incorporated Aaron's comments. In this patch, I moved the text track map from WebMediaPlayerImpl to TextRenderer. https://codereview.chromium.org/23702007/diff/24001/content/renderer/media/we... File content/renderer/media/webmediaplayer_impl.cc (right): https://codereview.chromium.org/23702007/diff/24001/content/renderer/media/we... content/renderer/media/webmediaplayer_impl.cc:973: text_track_map_[text_stream] = text_track.release(); On 2013/09/25 23:23:10, acolwell wrote: > I don't think you need the map here. You can hide the thread hopping business in > TextTrackImpl and let the renderer own the TextTrack instance. > > I think if you use the following signature instead you'd avoid the need for the > map here and avoid map lookups on each cue. It would also allow the TextRenderer > to control the text track lifetime instead of having to coordinate with this map > here. > typedef base::Callback<void(scoped_ptr<media::TextTrack>)> > AddTextTrack(media::TextKind kind, > const std::string& label, > const std::string& language, > const AddTextTrackDoneCB& done_cb) Done. https://codereview.chromium.org/23702007/diff/24001/content/renderer/media/we... content/renderer/media/webmediaplayer_impl.cc:1281: void WebMediaPlayerImpl::CueReady( On 2013/09/25 23:23:10, acolwell wrote: > You shouldn't need this. You can hide the thread hopping in TextTrackImpl and > the TextRenderer should just be able to interact with TextTrack objects. Done. https://codereview.chromium.org/23702007/diff/24001/media/base/demuxer.h File media/base/demuxer.h (right): https://codereview.chromium.org/23702007/diff/24001/media/base/demuxer.h#newc... media/base/demuxer.h:15: #include "media/base/text_track.h" On 2013/09/25 23:23:10, acolwell wrote: > nit: This doesn't appear to be needed. It contains the declaration of media::TextKind, which is used the signature of DemuxerHost::AddTextStream. https://codereview.chromium.org/23702007/diff/24001/media/base/demuxer.h#newc... media/base/demuxer.h:19: class DemuxerStream; On 2013/09/25 23:23:10, acolwell wrote: > nit: This shouldn't be needed since demuxer_stream.h is being included above. Done. https://codereview.chromium.org/23702007/diff/24001/media/base/mock_filters.h File media/base/mock_filters.h (right): https://codereview.chromium.org/23702007/diff/24001/media/base/mock_filters.h... media/base/mock_filters.h:107: class MockTextDecoder : public TextDecoder { On 2013/09/25 23:23:10, acolwell wrote: > nit: This class doesn't appear to be used anywhere. Please remove. Done. https://codereview.chromium.org/23702007/diff/24001/media/base/pipeline.cc File media/base/pipeline.cc (right): https://codereview.chromium.org/23702007/diff/24001/media/base/pipeline.cc#ne... media/base/pipeline.cc:24: #include "media/base/text_decoder.h" On 2013/09/25 23:23:10, acolwell wrote: > nit: It doesn't look like this is needed anywhere. Please remove. Done. https://codereview.chromium.org/23702007/diff/24001/media/base/pipeline.cc#ne... media/base/pipeline.cc:913: if (text_renderer_ && !text_ended_) On 2013/09/25 23:23:10, acolwell wrote: > I think you may need to add a HasTracks() method on text_renderer_ otherwise I > don't think playback will end for content w/o text tracks. Done. https://codereview.chromium.org/23702007/diff/24001/media/base/text_cue.h File media/base/text_cue.h (right): https://codereview.chromium.org/23702007/diff/24001/media/base/text_cue.h#new... media/base/text_cue.h:43: DISALLOW_COPY_AND_ASSIGN(TextCue); On 2013/09/25 23:23:10, acolwell wrote: > nit: s/DISALLOW_COPY_AND_ASSIGN/DISALLOW_IMPLICIT_CONSTRUCTORS Done.
https://codereview.chromium.org/23702007/diff/36001/content/renderer/media/te... File content/renderer/media/texttrack_impl.cc (right): https://codereview.chromium.org/23702007/diff/36001/content/renderer/media/te... content/renderer/media/texttrack_impl.cc:39: if (WebKit::WebInbandTextTrackClient* client = text_track_->client()) { I don't think this is safe. I think you have to do the text_track->client() eval inside OnAddCue() because I think the pointer could get cleared while the task is in flight. You should be able to simply pass text_track_.get() here since you know that this task will always run before OnRemoveTrack() task that is posted later. https://codereview.chromium.org/23702007/diff/36001/content/renderer/media/te... content/renderer/media/texttrack_impl.cc:64: WebInbandTextTrackImpl* text_track) { You should be able to make text_track a scoped_ptr<WebInbandTextTrackImpl> so you don't need the one below. You'll have to change the .release() to a .Pass() above as well to make this work. https://codereview.chromium.org/23702007/diff/36001/content/renderer/media/we... File content/renderer/media/webmediaplayer_impl.cc (right): https://codereview.chromium.org/23702007/diff/36001/content/renderer/media/we... content/renderer/media/webmediaplayer_impl.cc:40: #include "media/base/text_cue.h" nit: no longer needed. https://codereview.chromium.org/23702007/diff/36001/content/renderer/media/we... content/renderer/media/webmediaplayer_impl.cc:978: done_cb.Run(text_stream, text_track.Pass()); WebMediaPlayerImpl should not need to know about DemuxerStream. If you need it in lower layers, just use base::Bind to bind the value when creating done_cb. https://codereview.chromium.org/23702007/diff/36001/content/renderer/media/we... content/renderer/media/webmediaplayer_impl.cc:1067: // change the chunk demuxer to user the same mechanism as for ffmpeg. nit: s/user/use/. https://codereview.chromium.org/23702007/diff/36001/content/renderer/media/we... content/renderer/media/webmediaplayer_impl.cc:1150: new media::TextDecoderImpl(media_loop_)); Move this into the TextRenderer constructor since there is no need for this class to be aware of its existence. https://codereview.chromium.org/23702007/diff/36001/content/renderer/media/we... File content/renderer/media/webmediaplayer_impl.h (right): https://codereview.chromium.org/23702007/diff/36001/content/renderer/media/we... content/renderer/media/webmediaplayer_impl.h:23: #include <map> nit: no longer needed. https://codereview.chromium.org/23702007/diff/36001/content/renderer/media/we... content/renderer/media/webmediaplayer_impl.h:62: class TextCue; nit: Not needed https://codereview.chromium.org/23702007/diff/36001/content/renderer/media/we... content/renderer/media/webmediaplayer_impl.h:204: // to AddTextStream. I'd like this fixed in this CL. Now that OnAddTextStream provides a TextTrack, I think this should be pretty straight forward. https://codereview.chromium.org/23702007/diff/36001/media/base/pipeline.cc File media/base/pipeline.cc (right): https://codereview.chromium.org/23702007/diff/36001/media/base/pipeline.cc#ne... media/base/pipeline.cc:752: if (text_renderer_) nit: Add {} since this is a multi-line statement. https://codereview.chromium.org/23702007/diff/36001/media/base/text_decoder.h File media/base/text_decoder.h (right): https://codereview.chromium.org/23702007/diff/36001/media/base/text_decoder.h... media/base/text_decoder.h:17: class MEDIA_EXPORT TextDecoder { Since we only have one instance of a text decoder just remove this abstract interface and rename TextDecoderImpl to TextDecoder. https://codereview.chromium.org/23702007/diff/36001/media/base/text_renderer.cc File media/base/text_renderer.cc (right): https://codereview.chromium.org/23702007/diff/36001/media/base/text_renderer.... media/base/text_renderer.cc:66: if (val.second == kReadPending) { nit: Just use itr-> here and below. Val isn't really providing any extra clarity here. https://codereview.chromium.org/23702007/diff/36001/media/base/text_renderer.... media/base/text_renderer.cc:83: if (pending_read_count_ <= 0) { nit: Add DCHECK_GE(pending_read_count_, 0) above and just s/<=/==/ here since a negative count is a bug. https://codereview.chromium.org/23702007/diff/36001/media/base/text_renderer.... media/base/text_renderer.cc:89: state_ = kPausePending; nit: Consider using a ChangeState() helper method for all state transitions like ChunkDemuxer does. It makes debugging state transitions a lot easier. https://codereview.chromium.org/23702007/diff/36001/media/base/text_renderer.... media/base/text_renderer.cc:99: callback.Run(); You likely need to wait for any pending reads to complete before running the callback. Otherwise you won't know if the data returned is pre or post Flush(). https://codereview.chromium.org/23702007/diff/36001/media/base/text_renderer.... media/base/text_renderer.cc:109: if (pending_read_count_ <= 0) { nit: s/<=/==/ since a negative value is a sign of a bug. https://codereview.chromium.org/23702007/diff/36001/media/base/text_renderer.... media/base/text_renderer.cc:131: add_text_track_cb_.Run(text_stream, kind, label, language, done_cb); Yes. You should include text_stream in the Bind above. https://codereview.chromium.org/23702007/diff/36001/media/base/text_renderer.... media/base/text_renderer.cc:137: DCHECK(state_ != kUninitialized && state_ != kStopped); nit: Add << "state_ " << state so that it is easy to see what state we are actually in if this fails. Ditto for similar DCHECKs elsewhere or use multiple DCHECK_NE(state_, ...) instead. https://codereview.chromium.org/23702007/diff/36001/media/base/text_renderer.... media/base/text_renderer.cc:156: return (!text_track_map_.empty()); nit: remove unnecessary () https://codereview.chromium.org/23702007/diff/36001/media/base/text_renderer.... media/base/text_renderer.cc:181: if (pending_read_count_ > 0) Why do you need to worry about the pending_read_count_ here? You shouldn't have any pending reads if pending_eos_count_ == 0 right? It seems s/pending_read_count_/pending_eos_count_/ would be clearer and then just drop the <= conditional below. https://codereview.chromium.org/23702007/diff/36001/media/base/text_renderer.... media/base/text_renderer.cc:192: if (pending_read_count_ <= 0) { Do we really want to drop cues received while trying to pause? It seems like pause/play cycles could cause cues to go missing. https://codereview.chromium.org/23702007/diff/36001/media/base/text_renderer.... media/base/text_renderer.cc:210: default: nit: remove default so the compiler will warn us of unhandled states. https://codereview.chromium.org/23702007/diff/36001/media/base/text_renderer.... media/base/text_renderer.cc:215: if (!text_cue->text().empty()) { Why is a cue w/ empty text being skipped? Either these should not be generated by the demuxers or they should be allowed through and processed like all the other cues. I believe WebVTT allows cues w/ empty text so the later is probably appropriate. https://codereview.chromium.org/23702007/diff/36001/media/filters/ffmpeg_demu... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/23702007/diff/36001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:137: std::basic_string<uint8> side_data; nit:Use std::vector<uint8> to avoid any issues with the null terminator. https://codereview.chromium.org/23702007/diff/36001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:305: if (type_ != DemuxerStream::TEXT) nit: Convert this to a DCHECK since calling this on a non-TEXT track is a bug. https://codereview.chromium.org/23702007/diff/36001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:311: kind = kTextCaptions; nit: Just return here and below & drop the elses. https://codereview.chromium.org/23702007/diff/36001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:477: DCHECK_NE(kind, kTextNone); nit: You shouldn't need this DCHECK https://codereview.chromium.org/23702007/diff/36001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer.cc:844: if (demuxer_stream->type() != DemuxerStream::TEXT || text_enabled_) nit: It seems like the text_enabled_ check should be in OnFindStreamInfoDone() so you can avoid creating FFmpegDemuxerStream objects for text tracks instead of doing the check here. https://codereview.chromium.org/23702007/diff/36001/media/filters/ffmpeg_demu... File media/filters/ffmpeg_demuxer_unittest.cc (right): https://codereview.chromium.org/23702007/diff/36001/media/filters/ffmpeg_demu... media/filters/ffmpeg_demuxer_unittest.cc:307: TEST_F(FFmpegDemuxerTest, DISABLED_Initialize_MultitrackText) { nit: enable test. https://codereview.chromium.org/23702007/diff/36001/media/filters/pipeline_in... File media/filters/pipeline_integration_test.cc (right): https://codereview.chromium.org/23702007/diff/36001/media/filters/pipeline_in... media/filters/pipeline_integration_test.cc:1076: DISABLED_BasicPlayback_VP8_WebVTT_WebM) { nit: enable test https://codereview.chromium.org/23702007/diff/36001/media/filters/pipeline_in... File media/filters/pipeline_integration_test_base.cc (right): https://codereview.chromium.org/23702007/diff/36001/media/filters/pipeline_in... media/filters/pipeline_integration_test_base.cc:18: #include "media/filters/text_decoder_impl.h" nit: not used https://codereview.chromium.org/23702007/diff/36001/media/filters/pipeline_in... media/filters/pipeline_integration_test_base.cc:221: false, // don't enable inband text tracks nit: This should be true. We should be testing inband support in the integration tests. https://codereview.chromium.org/23702007/diff/36001/media/filters/pipeline_in... File media/filters/pipeline_integration_test_base.h (right): https://codereview.chromium.org/23702007/diff/36001/media/filters/pipeline_in... media/filters/pipeline_integration_test_base.h:30: class TextCue; nit: not needed. https://codereview.chromium.org/23702007/diff/36001/media/filters/pipeline_in... media/filters/pipeline_integration_test_base.h:106: TextTracks text_tracks_; nit: Not used anywhere. https://codereview.chromium.org/23702007/diff/36001/media/filters/text_decode... File media/filters/text_decoder_impl.cc (right): https://codereview.chromium.org/23702007/diff/36001/media/filters/text_decode... media/filters/text_decoder_impl.cc:42: void TextDecoderImpl::BufferReady( I don't think you need this class. This method seems to be the bulk of the logic here and you should be able to fold this method into the TextRenderer. You already are keeping track of pending reads there anyways so you could just convert that to mean pending reads on the DemuxerStream instead of on the TextDecoder. https://codereview.chromium.org/23702007/diff/36001/media/tools/demuxer_bench... File media/tools/demuxer_bench/demuxer_bench.cc (right): https://codereview.chromium.org/23702007/diff/36001/media/tools/demuxer_bench... media/tools/demuxer_bench/demuxer_bench.cc:202: add_text_track_cb, nit: I don't believe this compiles. I think you need a bool here. https://codereview.chromium.org/23702007/diff/36001/media/tools/player_x11/pl... File media/tools/player_x11/player_x11.cc (right): https://codereview.chromium.org/23702007/diff/36001/media/tools/player_x11/pl... media/tools/player_x11/player_x11.cc:283: base::Bind(&NeedKey), add_text_track_cb, new media::MediaLog())); nit: I don't believe this compiles. I think you need a bool here.
I incorporated Aaron's comments. This patch set includes the changes to the chunk demuxer. https://chromiumcodereview.appspot.com/23702007/diff/36001/content/renderer/m... File content/renderer/media/texttrack_impl.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/36001/content/renderer/m... content/renderer/media/texttrack_impl.cc:39: if (WebKit::WebInbandTextTrackClient* client = text_track_->client()) { On 2013/10/08 15:45:24, acolwell wrote: > I don't think this is safe. I think you have to do the text_track->client() eval > inside OnAddCue() because I think the pointer could get cleared while the task > is in flight. You should be able to simply pass text_track_.get() here since you > know that this task will always run before OnRemoveTrack() task that is posted > later. Done. https://chromiumcodereview.appspot.com/23702007/diff/36001/content/renderer/m... content/renderer/media/texttrack_impl.cc:64: WebInbandTextTrackImpl* text_track) { On 2013/10/08 15:45:24, acolwell wrote: > You should be able to make text_track a scoped_ptr<WebInbandTextTrackImpl> so > you don't need the one below. You'll have to change the .release() to a .Pass() > above as well to make this work. I had tried that, but the compiler complains about scoped_ptr not having a copy ctor. https://chromiumcodereview.appspot.com/23702007/diff/36001/content/renderer/m... File content/renderer/media/webmediaplayer_impl.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/36001/content/renderer/m... content/renderer/media/webmediaplayer_impl.cc:40: #include "media/base/text_cue.h" On 2013/10/08 15:45:24, acolwell wrote: > nit: no longer needed. Done. https://chromiumcodereview.appspot.com/23702007/diff/36001/content/renderer/m... content/renderer/media/webmediaplayer_impl.cc:978: done_cb.Run(text_stream, text_track.Pass()); On 2013/10/08 15:45:24, acolwell wrote: > WebMediaPlayerImpl should not need to know about DemuxerStream. If you need it > in lower layers, just use base::Bind to bind the value when creating done_cb. Done. https://chromiumcodereview.appspot.com/23702007/diff/36001/content/renderer/m... content/renderer/media/webmediaplayer_impl.cc:1067: // change the chunk demuxer to user the same mechanism as for ffmpeg. On 2013/10/08 15:45:24, acolwell wrote: > nit: s/user/use/. Done. https://chromiumcodereview.appspot.com/23702007/diff/36001/content/renderer/m... content/renderer/media/webmediaplayer_impl.cc:1150: new media::TextDecoderImpl(media_loop_)); On 2013/10/08 15:45:24, acolwell wrote: > Move this into the TextRenderer constructor since there is no need for this > class to be aware of its existence. Done. https://chromiumcodereview.appspot.com/23702007/diff/36001/content/renderer/m... File content/renderer/media/webmediaplayer_impl.h (right): https://chromiumcodereview.appspot.com/23702007/diff/36001/content/renderer/m... content/renderer/media/webmediaplayer_impl.h:23: #include <map> On 2013/10/08 15:45:24, acolwell wrote: > nit: no longer needed. Done. https://chromiumcodereview.appspot.com/23702007/diff/36001/content/renderer/m... content/renderer/media/webmediaplayer_impl.h:62: class TextCue; On 2013/10/08 15:45:24, acolwell wrote: > nit: Not needed Done. https://chromiumcodereview.appspot.com/23702007/diff/36001/content/renderer/m... content/renderer/media/webmediaplayer_impl.h:204: // to AddTextStream. On 2013/10/08 15:45:24, acolwell wrote: > I'd like this fixed in this CL. Now that OnAddTextStream provides a TextTrack, I > think this should be pretty straight forward. I modified the chunk demuxer and friends to handle the new schema. I'm a little fuzzy still about sources and source ids (you obviously know the Media Source API way better than I do), so there's a bit of chunk demuxer stream cleanup that needs to be added, but otherwise it's working. https://chromiumcodereview.appspot.com/23702007/diff/36001/media/base/pipelin... File media/base/pipeline.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/36001/media/base/pipelin... media/base/pipeline.cc:752: if (text_renderer_) On 2013/10/08 15:45:24, acolwell wrote: > nit: Add {} since this is a multi-line statement. Done. https://chromiumcodereview.appspot.com/23702007/diff/36001/media/base/text_de... File media/base/text_decoder.h (right): https://chromiumcodereview.appspot.com/23702007/diff/36001/media/base/text_de... media/base/text_decoder.h:17: class MEDIA_EXPORT TextDecoder { On 2013/10/08 15:45:24, acolwell wrote: > Since we only have one instance of a text decoder just remove this abstract > interface and rename TextDecoderImpl to TextDecoder. Done. https://chromiumcodereview.appspot.com/23702007/diff/36001/media/base/text_re... File media/base/text_renderer.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/36001/media/base/text_re... media/base/text_renderer.cc:66: if (val.second == kReadPending) { On 2013/10/08 15:45:24, acolwell wrote: > nit: Just use itr-> here and below. Val isn't really providing any extra clarity > here. Done. https://chromiumcodereview.appspot.com/23702007/diff/36001/media/base/text_re... media/base/text_renderer.cc:83: if (pending_read_count_ <= 0) { On 2013/10/08 15:45:24, acolwell wrote: > nit: Add DCHECK_GE(pending_read_count_, 0) above and just s/<=/==/ here since a > negative count is a bug. Done. https://chromiumcodereview.appspot.com/23702007/diff/36001/media/base/text_re... media/base/text_renderer.cc:89: state_ = kPausePending; On 2013/10/08 15:45:24, acolwell wrote: > nit: Consider using a ChangeState() helper method for all state transitions like > ChunkDemuxer does. It makes debugging state transitions a lot easier. Ack. https://chromiumcodereview.appspot.com/23702007/diff/36001/media/base/text_re... media/base/text_renderer.cc:99: callback.Run(); On 2013/10/08 15:45:24, acolwell wrote: > You likely need to wait for any pending reads to complete before running the > callback. Otherwise you won't know if the data returned is pre or post Flush(). No reads are pending during the kPaused or kEnded states. (Or is it the case that the flush request can arrive when state_ == kPausePending?) https://chromiumcodereview.appspot.com/23702007/diff/36001/media/base/text_re... media/base/text_renderer.cc:109: if (pending_read_count_ <= 0) { On 2013/10/08 15:45:24, acolwell wrote: > nit: s/<=/==/ since a negative value is a sign of a bug. Done. https://chromiumcodereview.appspot.com/23702007/diff/36001/media/base/text_re... media/base/text_renderer.cc:131: add_text_track_cb_.Run(text_stream, kind, label, language, done_cb); On 2013/10/08 15:45:24, acolwell wrote: > Yes. You should include text_stream in the Bind above. Done. https://chromiumcodereview.appspot.com/23702007/diff/36001/media/base/text_re... media/base/text_renderer.cc:137: DCHECK(state_ != kUninitialized && state_ != kStopped); On 2013/10/08 15:45:24, acolwell wrote: > nit: Add << "state_ " << state so that it is easy to see what state we are > actually in if this fails. Ditto for similar DCHECKs elsewhere or use multiple > DCHECK_NE(state_, ...) instead. Done. https://chromiumcodereview.appspot.com/23702007/diff/36001/media/base/text_re... media/base/text_renderer.cc:156: return (!text_track_map_.empty()); On 2013/10/08 15:45:24, acolwell wrote: > nit: remove unnecessary () Done. https://chromiumcodereview.appspot.com/23702007/diff/36001/media/base/text_re... media/base/text_renderer.cc:181: if (pending_read_count_ > 0) On 2013/10/08 15:45:24, acolwell wrote: > Why do you need to worry about the pending_read_count_ here? You shouldn't have > any pending reads if pending_eos_count_ == 0 right? It seems > s/pending_read_count_/pending_eos_count_/ would be clearer and then just drop > the <= conditional below. Done. https://chromiumcodereview.appspot.com/23702007/diff/36001/media/base/text_re... media/base/text_renderer.cc:192: if (pending_read_count_ <= 0) { On 2013/10/08 15:45:24, acolwell wrote: > Do we really want to drop cues received while trying to pause? It seems like > pause/play cycles could cause cues to go missing. Done. https://chromiumcodereview.appspot.com/23702007/diff/36001/media/base/text_re... media/base/text_renderer.cc:210: default: On 2013/10/08 15:45:24, acolwell wrote: > nit: remove default so the compiler will warn us of unhandled states. Done. https://chromiumcodereview.appspot.com/23702007/diff/36001/media/base/text_re... media/base/text_renderer.cc:215: if (!text_cue->text().empty()) { On 2013/10/08 15:45:24, acolwell wrote: > Why is a cue w/ empty text being skipped? Either these should not be generated > by the demuxers or they should be allowed through and processed like all the > other cues. I believe WebVTT allows cues w/ empty text so the later is probably > appropriate. Done. https://chromiumcodereview.appspot.com/23702007/diff/36001/media/filters/ffmp... File media/filters/ffmpeg_demuxer.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/36001/media/filters/ffmp... media/filters/ffmpeg_demuxer.cc:137: std::basic_string<uint8> side_data; On 2013/10/08 15:45:24, acolwell wrote: > nit:Use std::vector<uint8> to avoid any issues with the null terminator. Done. https://chromiumcodereview.appspot.com/23702007/diff/36001/media/filters/ffmp... media/filters/ffmpeg_demuxer.cc:305: if (type_ != DemuxerStream::TEXT) On 2013/10/08 15:45:24, acolwell wrote: > nit: Convert this to a DCHECK since calling this on a non-TEXT track is a bug. Done. https://chromiumcodereview.appspot.com/23702007/diff/36001/media/filters/ffmp... media/filters/ffmpeg_demuxer.cc:311: kind = kTextCaptions; On 2013/10/08 15:45:24, acolwell wrote: > nit: Just return here and below & drop the elses. Done. https://chromiumcodereview.appspot.com/23702007/diff/36001/media/filters/ffmp... media/filters/ffmpeg_demuxer.cc:477: DCHECK_NE(kind, kTextNone); On 2013/10/08 15:45:24, acolwell wrote: > nit: You shouldn't need this DCHECK Done. https://chromiumcodereview.appspot.com/23702007/diff/36001/media/filters/ffmp... media/filters/ffmpeg_demuxer.cc:844: if (demuxer_stream->type() != DemuxerStream::TEXT || text_enabled_) On 2013/10/08 15:45:24, acolwell wrote: > nit: It seems like the text_enabled_ check should be in OnFindStreamInfoDone() > so you can avoid creating FFmpegDemuxerStream objects for text tracks instead of > doing the check here. Done. https://chromiumcodereview.appspot.com/23702007/diff/36001/media/filters/ffmp... File media/filters/ffmpeg_demuxer_unittest.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/36001/media/filters/ffmp... media/filters/ffmpeg_demuxer_unittest.cc:307: TEST_F(FFmpegDemuxerTest, DISABLED_Initialize_MultitrackText) { On 2013/10/08 15:45:24, acolwell wrote: > nit: enable test. Done. https://chromiumcodereview.appspot.com/23702007/diff/36001/media/filters/pipe... File media/filters/pipeline_integration_test.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/36001/media/filters/pipe... media/filters/pipeline_integration_test.cc:1076: DISABLED_BasicPlayback_VP8_WebVTT_WebM) { On 2013/10/08 15:45:24, acolwell wrote: > nit: enable test Done. https://chromiumcodereview.appspot.com/23702007/diff/36001/media/filters/pipe... File media/filters/pipeline_integration_test_base.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/36001/media/filters/pipe... media/filters/pipeline_integration_test_base.cc:18: #include "media/filters/text_decoder_impl.h" On 2013/10/08 15:45:24, acolwell wrote: > nit: not used Done. https://chromiumcodereview.appspot.com/23702007/diff/36001/media/filters/pipe... media/filters/pipeline_integration_test_base.cc:221: false, // don't enable inband text tracks On 2013/10/08 15:45:24, acolwell wrote: > nit: This should be true. We should be testing inband support in the integration > tests. Done. https://chromiumcodereview.appspot.com/23702007/diff/36001/media/filters/pipe... File media/filters/pipeline_integration_test_base.h (right): https://chromiumcodereview.appspot.com/23702007/diff/36001/media/filters/pipe... media/filters/pipeline_integration_test_base.h:30: class TextCue; On 2013/10/08 15:45:24, acolwell wrote: > nit: not needed. Done. https://chromiumcodereview.appspot.com/23702007/diff/36001/media/filters/pipe... media/filters/pipeline_integration_test_base.h:106: TextTracks text_tracks_; On 2013/10/08 15:45:24, acolwell wrote: > nit: Not used anywhere. Done. https://chromiumcodereview.appspot.com/23702007/diff/36001/media/filters/text... File media/filters/text_decoder_impl.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/36001/media/filters/text... media/filters/text_decoder_impl.cc:42: void TextDecoderImpl::BufferReady( On 2013/10/08 15:45:24, acolwell wrote: > I don't think you need this class. This method seems to be the bulk of the logic > here and you should be able to fold this method into the TextRenderer. You > already are keeping track of pending reads there anyways so you could just > convert that to mean pending reads on the DemuxerStream instead of on the > TextDecoder. Done. https://chromiumcodereview.appspot.com/23702007/diff/36001/media/tools/demuxe... File media/tools/demuxer_bench/demuxer_bench.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/36001/media/tools/demuxe... media/tools/demuxer_bench/demuxer_bench.cc:202: add_text_track_cb, On 2013/10/08 15:45:24, acolwell wrote: > nit: I don't believe this compiles. I think you need a bool here. Done. https://chromiumcodereview.appspot.com/23702007/diff/36001/media/tools/player... File media/tools/player_x11/player_x11.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/36001/media/tools/player... media/tools/player_x11/player_x11.cc:283: base::Bind(&NeedKey), add_text_track_cb, new media::MediaLog())); On 2013/10/08 15:45:24, acolwell wrote: > nit: I don't believe this compiles. I think you need a bool here. Done.
Looking pretty good. https://chromiumcodereview.appspot.com/23702007/diff/49001/content/renderer/m... File content/renderer/media/webmediaplayer_impl.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/49001/content/renderer/m... content/renderer/media/webmediaplayer_impl.cc:955: const webkind_t webkind = static_cast<webkind_t>(kind); nit: Just use the actual type here instead of a typecast. You use 2 lines either way so just avoid the extra indirection. https://chromiumcodereview.appspot.com/23702007/diff/49001/content/renderer/m... content/renderer/media/webmediaplayer_impl.cc:960: new WebInbandTextTrackImpl(webkind, weblabel, weblanguage, nit: s/webkind/web_kind_/, s/weblabel/web_label/, s/weblanguage/web_language/ to be consistant w/ web_inband_text_track naming. https://chromiumcodereview.appspot.com/23702007/diff/49001/content/renderer/m... File content/renderer/media/webmediaplayer_impl.h (right): https://chromiumcodereview.appspot.com/23702007/diff/49001/content/renderer/m... content/renderer/media/webmediaplayer_impl.h:58: class DemuxerStream; nit: not needed. https://chromiumcodereview.appspot.com/23702007/diff/49001/media/base/pipelin... File media/base/pipeline.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/49001/media/base/pipelin... media/base/pipeline.cc:947: DCHECK(text_renderer_); nit: Remove this DCHECK since we'll NPE on the next line if this precondition isn't true. https://chromiumcodereview.appspot.com/23702007/diff/49001/media/base/text_re... File media/base/text_renderer.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/49001/media/base/text_re... media/base/text_renderer.cc:58: callback.Run(); It is safer to run the callback at the end of the method just to avoid any potential reentrancy issues. Running it at the end is also a better reflection that you are actually done starting playback. https://chromiumcodereview.appspot.com/23702007/diff/49001/media/base/text_re... media/base/text_renderer.cc:106: state_ == kPaused || I'm pretty sure you need to add kPausePending to this list. I'm pretty sure the pipeline won't wait for Pause() to complete before calling Stop(). https://chromiumcodereview.appspot.com/23702007/diff/49001/media/base/text_re... media/base/text_renderer.cc:142: state_ != kStopped) << "state_ " << state_; I believe it might be possible for state_ to be in kStopping or kStopped when this is run. You might just want to return early in those cases. https://chromiumcodereview.appspot.com/23702007/diff/49001/media/base/text_re... media/base/text_renderer.cc:170: nit: DCHECK_NE(status, DemuxerStream::kConfigChanged); https://chromiumcodereview.appspot.com/23702007/diff/49001/media/base/text_re... media/base/text_renderer.cc:173: CueReady(stream, NULL); I think there is a bug here. Aborted reads should not be able to trigger ended_cb_ to run. https://chromiumcodereview.appspot.com/23702007/diff/49001/media/base/text_re... media/base/text_renderer.cc:183: DCHECK(input.get()); nit: Remove since you would have NPE above it this wasn't true. https://chromiumcodereview.appspot.com/23702007/diff/49001/media/base/text_re... media/base/text_renderer.cc:262: DCHECK_NE(text_track, static_cast<media::TextTrack*>(NULL)); nit: Use DCHECK(text_track) instead. https://chromiumcodereview.appspot.com/23702007/diff/49001/media/base/text_re... media/base/text_renderer.cc:277: text_stream)); nit: return early & drop else https://chromiumcodereview.appspot.com/23702007/diff/49001/media/base/text_re... File media/base/text_renderer.h (right): https://chromiumcodereview.appspot.com/23702007/diff/49001/media/base/text_re... media/base/text_renderer.h:26: // Receives the decoded cues from the upstream decoder, and passes them nit: Update comment. There isn't a decoder anymore. https://chromiumcodereview.appspot.com/23702007/diff/49001/media/base/text_re... media/base/text_renderer.h:32: // |decoder| contains the TextDecoder to use when initializing. ditto https://chromiumcodereview.appspot.com/23702007/diff/49001/media/base/text_re... media/base/text_renderer.h:40: // Initialize the TextRenderer with the text streams from |demuxer|. nit: this comment appears to be out of date. https://chromiumcodereview.appspot.com/23702007/diff/49001/media/base/text_re... media/base/text_renderer.h:127: DISALLOW_COPY_AND_ASSIGN(TextRenderer); nit: s/COPY_AND_ASSIGN/IMPLICIT_CONSTRUCTORS/ https://chromiumcodereview.appspot.com/23702007/diff/49001/media/filters/chun... File media/filters/chunk_demuxer.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/49001/media/filters/chun... media/filters/chunk_demuxer.cc:477: if (buffers.empty()) nit: DCHECK(!buffers.empty()) instead. https://chromiumcodereview.appspot.com/23702007/diff/49001/media/filters/chun... media/filters/chunk_demuxer.cc:482: return true; nit: I think this should return false. Doesn't this mean that we have received cues for a track that we don't know about? This situation seems like it should trigger an error. https://chromiumcodereview.appspot.com/23702007/diff/49001/media/filters/chun... media/filters/chunk_demuxer.cc:489: void SourceState::OnNewTextTrack(int text_track_number, I think this may need to be folded into OnNewConfigs(). You need a global picture of all the text tracks provided in the headers so that you can determine whether the track numbers, kind, and language change. If there is one text track then the track number is allowed to change, but the kind and language are not. If there are more than one text track then the kind, language, and track numbers must all be the same. The number of tracks must also stay constant throughout the life of a SourceState object. You might want to consider creating a TextTrackConfig object similar to the AudioDecoderConfig that contains language, kind, and codec (i.e., WebVTT) so that it is easy to transport these values together and enables Matches() and IsValid() operations. https://chromiumcodereview.appspot.com/23702007/diff/49001/media/filters/chun... media/filters/chunk_demuxer.cc:699: } This needs to verify that kind and language haven't changed just like UpdateVideoConfig() and UpdateAudioConfig() verify that the codec and parameters like sample rate can't change from config to config. https://chromiumcodereview.appspot.com/23702007/diff/49001/media/filters/chun... media/filters/chunk_demuxer.cc:881: if (!enable_text_ || text_stream_set_.empty()) nit: I'd prefer to not return text tracks through this interface since it doesn't allow all tracks to be accessed and it represents the old way. Please add a DCHECK for the TEXT type and make sure the DemuxerStream interface has a comment indicating that |type| is not allowed to be TEXT. https://chromiumcodereview.appspot.com/23702007/diff/49001/media/filters/chun... media/filters/chunk_demuxer.cc:993: // TODO(matthewjheaney): delete the text streams associated with this id This needs to be addressed in this CL. If you let the SourceState object own the text DemuxerStreams instead of text_stream_set_, this should be pretty straight forward. You will also need a way to notify pipeline that these streams are being removed from the presentation. Adding the following signature to DemuxerHost and adding the necessary plumbing to Pipeline & TextRenderer should work. void RemoveTextStream(DemuxerStream* text_stream) https://chromiumcodereview.appspot.com/23702007/diff/49001/media/filters/chun... media/filters/chunk_demuxer.cc:1311: TextStreamSet::iterator it = text_stream_set_.begin(); This would go away if you let SourceState own the text DemuxerStreams. https://chromiumcodereview.appspot.com/23702007/diff/49001/media/filters/chun... media/filters/chunk_demuxer.cc:1486: for (TextStreamSet::iterator itr = text_stream_set_.begin(); nit: Consider iterating over the SourceState objects instead and have them each iterate over their DemuxerStreams. You'd likely be able to move the calls on audio_ & video_ into SourceState as well. https://chromiumcodereview.appspot.com/23702007/diff/49001/media/filters/chun... media/filters/chunk_demuxer.cc:1499: for (TextStreamSet::iterator itr = text_stream_set_.begin(); ditto for this & the other 2 methods below. https://chromiumcodereview.appspot.com/23702007/diff/49001/media/filters/ffmp... File media/filters/ffmpeg_demuxer.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/49001/media/filters/ffmp... media/filters/ffmpeg_demuxer.cc:17: //#include "base/stl_util.h" nit: remove https://chromiumcodereview.appspot.com/23702007/diff/49001/media/filters/ffmp... File media/filters/ffmpeg_demuxer_unittest.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/49001/media/filters/ffmp... media/filters/ffmpeg_demuxer_unittest.cc:329: stream = demuxer_->GetStream(DemuxerStream::TEXT); nit: Don't use this mechanism to verify that text tracks were parsed. Use AddTextStream() calls on the DemuxerHost instead. https://chromiumcodereview.appspot.com/23702007/diff/49001/media/filters/pipe... File media/filters/pipeline_integration_test_base.h (right): https://chromiumcodereview.appspot.com/23702007/diff/49001/media/filters/pipe... media/filters/pipeline_integration_test_base.h:27: class Demuxer; nit: remove
I added a TextTrackConfig and added it add a parameter to the OnNewConfigs callback. The implementation is incomplete but we can at least determine whether this is the approach you had in mind. https://chromiumcodereview.appspot.com/23702007/diff/49001/content/renderer/m... File content/renderer/media/webmediaplayer_impl.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/49001/content/renderer/m... content/renderer/media/webmediaplayer_impl.cc:955: const webkind_t webkind = static_cast<webkind_t>(kind); On 2013/10/14 20:42:24, acolwell wrote: > nit: Just use the actual type here instead of a typecast. You use 2 lines either > way so just avoid the extra indirection. Done. https://chromiumcodereview.appspot.com/23702007/diff/49001/content/renderer/m... content/renderer/media/webmediaplayer_impl.cc:960: new WebInbandTextTrackImpl(webkind, weblabel, weblanguage, On 2013/10/14 20:42:24, acolwell wrote: > nit: s/webkind/web_kind_/, s/weblabel/web_label/, s/weblanguage/web_language/ to > be consistant w/ web_inband_text_track naming. Done. https://chromiumcodereview.appspot.com/23702007/diff/49001/content/renderer/m... File content/renderer/media/webmediaplayer_impl.h (right): https://chromiumcodereview.appspot.com/23702007/diff/49001/content/renderer/m... content/renderer/media/webmediaplayer_impl.h:58: class DemuxerStream; On 2013/10/14 20:42:24, acolwell wrote: > nit: not needed. Done. https://chromiumcodereview.appspot.com/23702007/diff/49001/media/base/pipelin... File media/base/pipeline.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/49001/media/base/pipelin... media/base/pipeline.cc:947: DCHECK(text_renderer_); On 2013/10/14 20:42:24, acolwell wrote: > nit: Remove this DCHECK since we'll NPE on the next line if this precondition > isn't true. Done. https://chromiumcodereview.appspot.com/23702007/diff/49001/media/base/text_re... File media/base/text_renderer.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/49001/media/base/text_re... media/base/text_renderer.cc:58: callback.Run(); On 2013/10/14 20:42:24, acolwell wrote: > It is safer to run the callback at the end of the method just to avoid any > potential reentrancy issues. Running it at the end is also a better reflection > that you are actually done starting playback. Done. https://chromiumcodereview.appspot.com/23702007/diff/49001/media/base/text_re... media/base/text_renderer.cc:58: callback.Run(); On 2013/10/14 20:42:24, acolwell wrote: > It is safer to run the callback at the end of the method just to avoid any > potential reentrancy issues. Running it at the end is also a better reflection > that you are actually done starting playback. Done. https://chromiumcodereview.appspot.com/23702007/diff/49001/media/base/text_re... media/base/text_renderer.cc:106: state_ == kPaused || On 2013/10/14 20:42:24, acolwell wrote: > I'm pretty sure you need to add kPausePending to this list. I'm pretty sure the > pipeline won't wait for Pause() to complete before calling Stop(). Done. https://chromiumcodereview.appspot.com/23702007/diff/49001/media/base/text_re... media/base/text_renderer.cc:142: state_ != kStopped) << "state_ " << state_; On 2013/10/14 20:42:24, acolwell wrote: > I believe it might be possible for state_ to be in kStopping or kStopped when > this is run. You might just want to return early in those cases. Done. https://chromiumcodereview.appspot.com/23702007/diff/49001/media/base/text_re... media/base/text_renderer.cc:170: On 2013/10/14 20:42:24, acolwell wrote: > nit: DCHECK_NE(status, DemuxerStream::kConfigChanged); Done. https://chromiumcodereview.appspot.com/23702007/diff/49001/media/base/text_re... media/base/text_renderer.cc:173: CueReady(stream, NULL); On 2013/10/14 20:42:24, acolwell wrote: > I think there is a bug here. Aborted reads should not be able to trigger > ended_cb_ to run. Done. https://chromiumcodereview.appspot.com/23702007/diff/49001/media/base/text_re... media/base/text_renderer.cc:183: DCHECK(input.get()); On 2013/10/14 20:42:24, acolwell wrote: > nit: Remove since you would have NPE above it this wasn't true. Done. https://chromiumcodereview.appspot.com/23702007/diff/49001/media/base/text_re... media/base/text_renderer.cc:262: DCHECK_NE(text_track, static_cast<media::TextTrack*>(NULL)); On 2013/10/14 20:42:24, acolwell wrote: > nit: Use DCHECK(text_track) instead. Done. https://chromiumcodereview.appspot.com/23702007/diff/49001/media/base/text_re... media/base/text_renderer.cc:277: text_stream)); On 2013/10/14 20:42:24, acolwell wrote: > nit: return early & drop else Done. https://chromiumcodereview.appspot.com/23702007/diff/49001/media/base/text_re... File media/base/text_renderer.h (right): https://chromiumcodereview.appspot.com/23702007/diff/49001/media/base/text_re... media/base/text_renderer.h:26: // Receives the decoded cues from the upstream decoder, and passes them On 2013/10/14 20:42:24, acolwell wrote: > nit: Update comment. There isn't a decoder anymore. Done. https://chromiumcodereview.appspot.com/23702007/diff/49001/media/base/text_re... media/base/text_renderer.h:32: // |decoder| contains the TextDecoder to use when initializing. On 2013/10/14 20:42:24, acolwell wrote: > ditto Done. https://chromiumcodereview.appspot.com/23702007/diff/49001/media/base/text_re... media/base/text_renderer.h:40: // Initialize the TextRenderer with the text streams from |demuxer|. On 2013/10/14 20:42:24, acolwell wrote: > nit: this comment appears to be out of date. Done. https://chromiumcodereview.appspot.com/23702007/diff/49001/media/base/text_re... media/base/text_renderer.h:40: // Initialize the TextRenderer with the text streams from |demuxer|. On 2013/10/14 20:42:24, acolwell wrote: > nit: this comment appears to be out of date. Done. https://chromiumcodereview.appspot.com/23702007/diff/49001/media/base/text_re... media/base/text_renderer.h:127: DISALLOW_COPY_AND_ASSIGN(TextRenderer); On 2013/10/14 20:42:24, acolwell wrote: > nit: s/COPY_AND_ASSIGN/IMPLICIT_CONSTRUCTORS/ Done. https://chromiumcodereview.appspot.com/23702007/diff/49001/media/base/text_re... media/base/text_renderer.h:127: DISALLOW_COPY_AND_ASSIGN(TextRenderer); On 2013/10/14 20:42:24, acolwell wrote: > nit: s/COPY_AND_ASSIGN/IMPLICIT_CONSTRUCTORS/ Done. https://chromiumcodereview.appspot.com/23702007/diff/49001/media/filters/chun... File media/filters/chunk_demuxer.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/49001/media/filters/chun... media/filters/chunk_demuxer.cc:477: if (buffers.empty()) On 2013/10/14 20:42:24, acolwell wrote: > nit: DCHECK(!buffers.empty()) instead. Done. https://chromiumcodereview.appspot.com/23702007/diff/49001/media/filters/chun... media/filters/chunk_demuxer.cc:477: if (buffers.empty()) On 2013/10/14 20:42:24, acolwell wrote: > nit: DCHECK(!buffers.empty()) instead. Done. https://chromiumcodereview.appspot.com/23702007/diff/49001/media/filters/chun... media/filters/chunk_demuxer.cc:482: return true; On 2013/10/14 20:42:24, acolwell wrote: > nit: I think this should return false. Doesn't this mean that we have received > cues for a track that we don't know about? This situation seems like it should > trigger an error. Done. https://chromiumcodereview.appspot.com/23702007/diff/49001/media/filters/chun... media/filters/chunk_demuxer.cc:489: void SourceState::OnNewTextTrack(int text_track_number, On 2013/10/14 20:42:24, acolwell wrote: > I think this may need to be folded into OnNewConfigs(). You need a global > picture of all the text tracks provided in the headers so that you can determine > whether the track numbers, kind, and language change. If there is one text track > then the track number is allowed to change, but the kind and language are not. > If there are more than one text track then the kind, language, and track numbers > must all be the same. The number of tracks must also stay constant throughout > the life of a SourceState object. > > You might want to consider creating a TextTrackConfig object similar to the > AudioDecoderConfig that contains language, kind, and codec (i.e., WebVTT) so > that it is easy to transport these values together and enables Matches() and > IsValid() operations. I did an incomplete implementation. You can check it out and make sure I'm headed in the right direction. https://chromiumcodereview.appspot.com/23702007/diff/49001/media/filters/chun... media/filters/chunk_demuxer.cc:699: } On 2013/10/14 20:42:24, acolwell wrote: > This needs to verify that kind and language haven't changed just like > UpdateVideoConfig() and UpdateAudioConfig() verify that the codec and parameters > like sample rate can't change from config to config. See if I got OnNewConfigs right, then I can adjust this as necessary. I am a little unclear about going from no streams (as when the source state object is first created) to non-zero streams (e.g. the very first parse of a webm stream). It's one thing to change the configs, but I'm talking about first-time initialization. https://chromiumcodereview.appspot.com/23702007/diff/49001/media/filters/chun... media/filters/chunk_demuxer.cc:881: if (!enable_text_ || text_stream_set_.empty()) On 2013/10/14 20:42:24, acolwell wrote: > nit: I'd prefer to not return text tracks through this interface since it > doesn't allow all tracks to be accessed and it represents the old way. Please > add a DCHECK for the TEXT type and make sure the DemuxerStream interface has a > comment indicating that |type| is not allowed to be TEXT. Done. https://chromiumcodereview.appspot.com/23702007/diff/49001/media/filters/chun... media/filters/chunk_demuxer.cc:993: // TODO(matthewjheaney): delete the text streams associated with this id On 2013/10/14 20:42:24, acolwell wrote: > This needs to be addressed in this CL. If you let the SourceState object own the > text DemuxerStreams instead of text_stream_set_, this should be pretty straight > forward. > > You will also need a way to notify pipeline that these streams are being removed > from the presentation. Adding the following signature to DemuxerHost and adding > the necessary plumbing to Pipeline & TextRenderer should work. > > void RemoveTextStream(DemuxerStream* text_stream) I did a partial implementation. You can see if I'm headed in the right direction. https://chromiumcodereview.appspot.com/23702007/diff/49001/media/filters/chun... media/filters/chunk_demuxer.cc:1311: TextStreamSet::iterator it = text_stream_set_.begin(); On 2013/10/14 20:42:24, acolwell wrote: > This would go away if you let SourceState own the text DemuxerStreams. Done. https://chromiumcodereview.appspot.com/23702007/diff/49001/media/filters/chun... media/filters/chunk_demuxer.cc:1486: for (TextStreamSet::iterator itr = text_stream_set_.begin(); On 2013/10/14 20:42:24, acolwell wrote: > nit: Consider iterating over the SourceState objects instead and have them each > iterate over their DemuxerStreams. You'd likely be able to move the calls on > audio_ & video_ into SourceState as well. Done. https://chromiumcodereview.appspot.com/23702007/diff/49001/media/filters/chun... media/filters/chunk_demuxer.cc:1499: for (TextStreamSet::iterator itr = text_stream_set_.begin(); On 2013/10/14 20:42:24, acolwell wrote: > ditto for this & the other 2 methods below. Done. https://chromiumcodereview.appspot.com/23702007/diff/49001/media/filters/ffmp... File media/filters/ffmpeg_demuxer.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/49001/media/filters/ffmp... media/filters/ffmpeg_demuxer.cc:17: //#include "base/stl_util.h" On 2013/10/14 20:42:24, acolwell wrote: > nit: remove Done. https://chromiumcodereview.appspot.com/23702007/diff/49001/media/filters/ffmp... File media/filters/ffmpeg_demuxer_unittest.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/49001/media/filters/ffmp... media/filters/ffmpeg_demuxer_unittest.cc:329: stream = demuxer_->GetStream(DemuxerStream::TEXT); On 2013/10/14 20:42:24, acolwell wrote: > nit: Don't use this mechanism to verify that text tracks were parsed. Use > AddTextStream() calls on the DemuxerHost instead. Done. https://chromiumcodereview.appspot.com/23702007/diff/49001/media/filters/pipe... File media/filters/pipeline_integration_test_base.h (right): https://chromiumcodereview.appspot.com/23702007/diff/49001/media/filters/pipe... media/filters/pipeline_integration_test_base.h:27: class Demuxer; On 2013/10/14 20:42:24, acolwell wrote: > nit: remove Done.
https://codereview.chromium.org/23702007/diff/61001/content/renderer/media/te... File content/renderer/media/texttrack_impl.cc (right): https://codereview.chromium.org/23702007/diff/61001/content/renderer/media/te... content/renderer/media/texttrack_impl.cc:19: WebInbandTextTrackImpl* text_track) nit: make scoped_ptr<WebInbandTextTrackImpl> https://codereview.chromium.org/23702007/diff/61001/content/renderer/media/te... content/renderer/media/texttrack_impl.cc:31: text_track_.release())); You need to use base::Passed(&text_track_) here to make the compiler happy w/ the scoped_ptr. https://codereview.chromium.org/23702007/diff/61001/content/renderer/media/te... content/renderer/media/texttrack_impl.cc:65: scoped_ptr<WebInbandTextTrackImpl> auto_delete(text_track); remove. https://codereview.chromium.org/23702007/diff/61001/content/renderer/media/te... File content/renderer/media/texttrack_impl.h (right): https://codereview.chromium.org/23702007/diff/61001/content/renderer/media/te... content/renderer/media/texttrack_impl.h:51: WebInbandTextTrackImpl* text_track); nit: Use scoped_ptr for text_track. https://codereview.chromium.org/23702007/diff/61001/content/renderer/media/we... File content/renderer/media/webmediaplayer_impl.cc (right): https://codereview.chromium.org/23702007/diff/61001/content/renderer/media/we... content/renderer/media/webmediaplayer_impl.cc:959: WebInbandTextTrackImpl* const web_inband_text_track = nit: use scoped_ptr here. https://codereview.chromium.org/23702007/diff/61001/content/renderer/media/we... content/renderer/media/webmediaplayer_impl.cc:1129: BIND_TO_RENDER_LOOP(&WebMediaPlayerImpl::OnAddTextTrack); nit: Inline below. https://codereview.chromium.org/23702007/diff/61001/content/renderer/media/we... File content/renderer/media/webmediaplayer_impl.h (right): https://codereview.chromium.org/23702007/diff/61001/content/renderer/media/we... content/renderer/media/webmediaplayer_impl.h:200: const std::string& language, nit: Use const TextTrackConfig& here instead of the 3 parameters. https://codereview.chromium.org/23702007/diff/61001/media/base/demuxer.h File media/base/demuxer.h (right): https://codereview.chromium.org/23702007/diff/61001/media/base/demuxer.h#newc... media/base/demuxer.h:31: TextKind kind, nit: Use const TextTrackConfig& here instead of the 3 parameters. https://codereview.chromium.org/23702007/diff/61001/media/base/demuxer_stream.h File media/base/demuxer_stream.h (right): https://codereview.chromium.org/23702007/diff/61001/media/base/demuxer_stream... media/base/demuxer_stream.h:69: virtual TextTrackConfig text_track_config() = 0; nit: Is this really needed on the DemuxerStream interface? I don't see anything outside of ChunkDemuxer needing it. https://codereview.chromium.org/23702007/diff/61001/media/base/mock_demuxer_h... File media/base/mock_demuxer_host.h (right): https://codereview.chromium.org/23702007/diff/61001/media/base/mock_demuxer_h... media/base/mock_demuxer_host.h:37: TextStreamMap text_streams_; nit: I don't think you need the map here. The mock isn't verifying anything. I think these should just be MOCK_METHODs like the others and people can specify the expected DemuxerStreams when writing tests if they actually care. https://codereview.chromium.org/23702007/diff/61001/media/base/pipeline.cc File media/base/pipeline.cc (right): https://codereview.chromium.org/23702007/diff/61001/media/base/pipeline.cc#ne... media/base/pipeline.cc:302: base::AutoLock auto_lock(lock_); I don't think you need to lock here since you are just posting a task to the message_loop_. https://codereview.chromium.org/23702007/diff/61001/media/base/pipeline.cc#ne... media/base/pipeline.cc:309: DCHECK(message_loop_->BelongsToCurrentThread()); I think you'll need to PostTask() here too since tracks can get removed by removing a SourceBuffer which happens on the renderer thread. https://codereview.chromium.org/23702007/diff/61001/media/base/pipeline.h File media/base/pipeline.h (right): https://codereview.chromium.org/23702007/diff/61001/media/base/pipeline.h#new... media/base/pipeline.h:304: TextKind kind, nit: Use const TextTrackConfig& here instead. https://codereview.chromium.org/23702007/diff/61001/media/base/stream_parser.h File media/base/stream_parser.h (right): https://codereview.chromium.org/23702007/diff/61001/media/base/stream_parser.... media/base/stream_parser.h:65: // First parameter - The (number of the) text track to which these cues will nit: s/number/id & drop the parenthesis. https://codereview.chromium.org/23702007/diff/61001/media/base/stream_parser.... media/base/stream_parser.h:71: typedef base::Callback<bool(int track_number, nit: remove name here https://codereview.chromium.org/23702007/diff/61001/media/base/text_renderer.cc File media/base/text_renderer.cc (right): https://codereview.chromium.org/23702007/diff/61001/media/base/text_renderer.... media/base/text_renderer.cc:173: DCHECK(!input.get()); nit: Do you really need the .get() here? https://codereview.chromium.org/23702007/diff/61001/media/base/text_renderer.... media/base/text_renderer.cc:179: --pending_eos_count_; Why is the eos count being decremented here? An abort is not an end of stream situation. It is just a precursor to a seek or a stop. https://codereview.chromium.org/23702007/diff/61001/media/base/text_renderer.... media/base/text_renderer.cc:259: case kUninitialized: Missing kEnded from this switch https://codereview.chromium.org/23702007/diff/61001/media/base/text_renderer.... media/base/text_renderer.cc:274: text_cue->id(), nit: Fix indent https://codereview.chromium.org/23702007/diff/61001/media/base/text_renderer.... media/base/text_renderer.cc:288: state_ = kPaused; DCHECK_EQ(state_, kPausePending) << " state_ << " << state_; https://codereview.chromium.org/23702007/diff/61001/media/base/text_renderer.h File media/base/text_renderer.h (right): https://codereview.chromium.org/23702007/diff/61001/media/base/text_renderer.... media/base/text_renderer.h:39: // Establish representation invariants for the TextRenderer. nit: Remove this comment since it seems unnecessarily cryptic. https://codereview.chromium.org/23702007/diff/61001/media/base/text_renderer.... media/base/text_renderer.h:62: TextKind kind, nit: Use const TextTrackConfig& here. https://codereview.chromium.org/23702007/diff/61001/media/base/text_renderer.... media/base/text_renderer.h:123: TextTrackMap text_track_map_; nit: Create a TextTrackState object that holds the ReadState & TextTrack so you only have to maintain 1 map. https://codereview.chromium.org/23702007/diff/61001/media/base/text_track.cc File media/base/text_track.cc (right): https://codereview.chromium.org/23702007/diff/61001/media/base/text_track.cc#... media/base/text_track.cc:17: label_(label_), nit: s/_// in second label_ https://codereview.chromium.org/23702007/diff/61001/media/base/text_track.cc#... media/base/text_track.cc:18: language_(language_) { ditto https://codereview.chromium.org/23702007/diff/61001/media/base/text_track.cc#... media/base/text_track.cc:22: if (config.kind_ != kind_) nit: Merge all this into a single return statement. https://codereview.chromium.org/23702007/diff/61001/media/base/text_track.h File media/base/text_track.h (right): https://codereview.chromium.org/23702007/diff/61001/media/base/text_track.h#n... media/base/text_track.h:40: (TextKind kind, nit: Use const TextTrackConfig& here instead. https://codereview.chromium.org/23702007/diff/61001/media/base/text_track.h#n... media/base/text_track.h:54: Add kind(), label(), language() accessors. https://codereview.chromium.org/23702007/diff/61001/media/filters/chunk_demux... File media/filters/chunk_demuxer.cc (right): https://codereview.chromium.org/23702007/diff/61001/media/filters/chunk_demux... media/filters/chunk_demuxer.cc:72: void TextSeek(TimeDelta); nit: Specify parameter name here. https://codereview.chromium.org/23702007/diff/61001/media/filters/chunk_demux... media/filters/chunk_demuxer.cc:274: STLDeleteValues(&text_stream_map_); I believe you may need to call Shutdown() on each of the streams to cancel any pending reads they might have. https://codereview.chromium.org/23702007/diff/61001/media/filters/chunk_demux... media/filters/chunk_demuxer.cc:366: const TextTrackConfigMap& text_config) { nit: s/test_config/text_configs/ https://codereview.chromium.org/23702007/diff/61001/media/filters/chunk_demux... media/filters/chunk_demuxer.cc:436: const TextStreamMap::size_type text_count = text_stream_map_.size(); nit: Please just use size_t here. https://codereview.chromium.org/23702007/diff/61001/media/filters/chunk_demux... media/filters/chunk_demuxer.cc:448: // TODO(matthewjheaney): replace old with new I think this is just text_stream_map_.clear(). text_stream_map_[config_itr->first] = text_stream; Right? https://codereview.chromium.org/23702007/diff/61001/media/filters/chunk_demux... media/filters/chunk_demuxer.cc:456: success &= false; nit: Break out of the loop here and then drop the else below & deindent. https://codereview.chromium.org/23702007/diff/61001/media/filters/chunk_demux... media/filters/chunk_demuxer.cc:467: // TODO(matthewjheaney): if all configs match, then replace old with new I don't think you need to do anything here. If they all match then everything should be able to proceed as normal right? https://codereview.chromium.org/23702007/diff/61001/media/filters/chunk_demux... media/filters/chunk_demuxer.cc:755: if (!stream_) { Since we don't update TextTrackConfigs, you should be able to just DCHECK(!stream_) here instead. https://codereview.chromium.org/23702007/diff/61001/media/filters/chunk_demux... media/filters/chunk_demuxer.cc:1055: // TODO(matthewjheaney): delete the text streams associated with this id nit: No longer needed since the streams get deleted with the SourceState. https://codereview.chromium.org/23702007/diff/61001/media/filters/chunk_demux... media/filters/chunk_demuxer.cc:1476: #if 0 remove this block if it is no longer needed. https://codereview.chromium.org/23702007/diff/61001/media/filters/chunk_demux... media/filters/chunk_demuxer.cc:1536: audio_->StartReturningData(); Is there any reason the calls on audio_ and video_ can't be moved into SourceState as well? Ditto for the similar methods below. https://codereview.chromium.org/23702007/diff/61001/media/filters/chunk_demux... File media/filters/chunk_demuxer.h (right): https://codereview.chromium.org/23702007/diff/61001/media/filters/chunk_demux... media/filters/chunk_demuxer.h:175: TextKind kind, nit: Use const TextTrackConfig& here. https://codereview.chromium.org/23702007/diff/61001/media/filters/source_buff... File media/filters/source_buffer_stream.h (right): https://codereview.chromium.org/23702007/diff/61001/media/filters/source_buff... media/filters/source_buffer_stream.h:320: // TODO(matthewjheaney): does this need to be a vector? no. since we don't allow config changes on text tracks right now. https://codereview.chromium.org/23702007/diff/61001/media/webm/webm_cluster_p... File media/webm/webm_cluster_parser.cc (right): https://codereview.chromium.org/23702007/diff/61001/media/webm/webm_cluster_p... media/webm/webm_cluster_parser.cc:359: std::vector<char> side_data; The side_data construction should be in a helper method so that the FFmpeg code and this code can use the same logic.
Incorporated Aaron's comments. Aaron answered a few questions that I had about changes to the chunk demuxer, and so those issues are resolved in this patch set. https://chromiumcodereview.appspot.com/23702007/diff/61001/content/renderer/m... File content/renderer/media/texttrack_impl.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/61001/content/renderer/m... content/renderer/media/texttrack_impl.cc:19: WebInbandTextTrackImpl* text_track) On 2013/10/21 20:10:40, acolwell wrote: > nit: make scoped_ptr<WebInbandTextTrackImpl> Done. https://chromiumcodereview.appspot.com/23702007/diff/61001/content/renderer/m... content/renderer/media/texttrack_impl.cc:31: text_track_.release())); On 2013/10/21 20:10:40, acolwell wrote: > You need to use base::Passed(&text_track_) here to make the compiler happy w/ > the scoped_ptr. Done. https://chromiumcodereview.appspot.com/23702007/diff/61001/content/renderer/m... content/renderer/media/texttrack_impl.cc:65: scoped_ptr<WebInbandTextTrackImpl> auto_delete(text_track); On 2013/10/21 20:10:40, acolwell wrote: > remove. Done. https://chromiumcodereview.appspot.com/23702007/diff/61001/content/renderer/m... File content/renderer/media/texttrack_impl.h (right): https://chromiumcodereview.appspot.com/23702007/diff/61001/content/renderer/m... content/renderer/media/texttrack_impl.h:51: WebInbandTextTrackImpl* text_track); On 2013/10/21 20:10:40, acolwell wrote: > nit: Use scoped_ptr for text_track. Done. https://chromiumcodereview.appspot.com/23702007/diff/61001/content/renderer/m... File content/renderer/media/webmediaplayer_impl.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/61001/content/renderer/m... content/renderer/media/webmediaplayer_impl.cc:959: WebInbandTextTrackImpl* const web_inband_text_track = On 2013/10/21 20:10:40, acolwell wrote: > nit: use scoped_ptr here. Done. https://chromiumcodereview.appspot.com/23702007/diff/61001/content/renderer/m... content/renderer/media/webmediaplayer_impl.cc:1129: BIND_TO_RENDER_LOOP(&WebMediaPlayerImpl::OnAddTextTrack); On 2013/10/21 20:10:40, acolwell wrote: > nit: Inline below. Done. https://chromiumcodereview.appspot.com/23702007/diff/61001/content/renderer/m... File content/renderer/media/webmediaplayer_impl.h (right): https://chromiumcodereview.appspot.com/23702007/diff/61001/content/renderer/m... content/renderer/media/webmediaplayer_impl.h:200: const std::string& language, On 2013/10/21 20:10:40, acolwell wrote: > nit: Use const TextTrackConfig& here instead of the 3 parameters. Done. https://chromiumcodereview.appspot.com/23702007/diff/61001/media/base/demuxer.h File media/base/demuxer.h (right): https://chromiumcodereview.appspot.com/23702007/diff/61001/media/base/demuxer... media/base/demuxer.h:31: TextKind kind, On 2013/10/21 20:10:40, acolwell wrote: > nit: Use const TextTrackConfig& here instead of the 3 parameters. Done. https://chromiumcodereview.appspot.com/23702007/diff/61001/media/base/demuxer... File media/base/demuxer_stream.h (right): https://chromiumcodereview.appspot.com/23702007/diff/61001/media/base/demuxer... media/base/demuxer_stream.h:69: virtual TextTrackConfig text_track_config() = 0; On 2013/10/21 20:10:40, acolwell wrote: > nit: Is this really needed on the DemuxerStream interface? I don't see anything > outside of ChunkDemuxer needing it. Done. https://chromiumcodereview.appspot.com/23702007/diff/61001/media/base/mock_de... File media/base/mock_demuxer_host.h (right): https://chromiumcodereview.appspot.com/23702007/diff/61001/media/base/mock_de... media/base/mock_demuxer_host.h:37: TextStreamMap text_streams_; On 2013/10/21 20:10:40, acolwell wrote: > nit: I don't think you need the map here. The mock isn't verifying anything. I > think these should just be MOCK_METHODs like the others and people can specify > the expected DemuxerStreams when writing tests if they actually care. Done. https://chromiumcodereview.appspot.com/23702007/diff/61001/media/base/pipelin... File media/base/pipeline.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/61001/media/base/pipelin... media/base/pipeline.cc:302: base::AutoLock auto_lock(lock_); On 2013/10/21 20:10:40, acolwell wrote: > I don't think you need to lock here since you are just posting a task to the > message_loop_. Done. https://chromiumcodereview.appspot.com/23702007/diff/61001/media/base/pipelin... media/base/pipeline.cc:309: DCHECK(message_loop_->BelongsToCurrentThread()); On 2013/10/21 20:10:40, acolwell wrote: > I think you'll need to PostTask() here too since tracks can get removed by > removing a SourceBuffer which happens on the renderer thread. Done. https://chromiumcodereview.appspot.com/23702007/diff/61001/media/base/pipeline.h File media/base/pipeline.h (right): https://chromiumcodereview.appspot.com/23702007/diff/61001/media/base/pipelin... media/base/pipeline.h:304: TextKind kind, On 2013/10/21 20:10:40, acolwell wrote: > nit: Use const TextTrackConfig& here instead. Done. https://chromiumcodereview.appspot.com/23702007/diff/61001/media/base/stream_... File media/base/stream_parser.h (right): https://chromiumcodereview.appspot.com/23702007/diff/61001/media/base/stream_... media/base/stream_parser.h:65: // First parameter - The (number of the) text track to which these cues will On 2013/10/21 20:10:40, acolwell wrote: > nit: s/number/id & drop the parenthesis. Done. https://chromiumcodereview.appspot.com/23702007/diff/61001/media/base/stream_... media/base/stream_parser.h:71: typedef base::Callback<bool(int track_number, On 2013/10/21 20:10:40, acolwell wrote: > nit: remove name here Done. https://chromiumcodereview.appspot.com/23702007/diff/61001/media/base/text_re... File media/base/text_renderer.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/61001/media/base/text_re... media/base/text_renderer.cc:173: DCHECK(!input.get()); On 2013/10/21 20:10:40, acolwell wrote: > nit: Do you really need the .get() here? Done. https://chromiumcodereview.appspot.com/23702007/diff/61001/media/base/text_re... media/base/text_renderer.cc:179: --pending_eos_count_; On 2013/10/21 20:10:40, acolwell wrote: > Why is the eos count being decremented here? An abort is not an end of stream > situation. It is just a precursor to a seek or a stop. Done. https://chromiumcodereview.appspot.com/23702007/diff/61001/media/base/text_re... media/base/text_renderer.cc:259: case kUninitialized: On 2013/10/21 20:10:40, acolwell wrote: > Missing kEnded from this switch Done. https://chromiumcodereview.appspot.com/23702007/diff/61001/media/base/text_re... media/base/text_renderer.cc:274: text_cue->id(), On 2013/10/21 20:10:40, acolwell wrote: > nit: Fix indent Done. https://chromiumcodereview.appspot.com/23702007/diff/61001/media/base/text_re... media/base/text_renderer.cc:288: state_ = kPaused; On 2013/10/21 20:10:40, acolwell wrote: > DCHECK_EQ(state_, kPausePending) << " state_ << " << state_; Done. https://chromiumcodereview.appspot.com/23702007/diff/61001/media/base/text_re... File media/base/text_renderer.h (right): https://chromiumcodereview.appspot.com/23702007/diff/61001/media/base/text_re... media/base/text_renderer.h:39: // Establish representation invariants for the TextRenderer. On 2013/10/21 20:10:40, acolwell wrote: > nit: Remove this comment since it seems unnecessarily cryptic. Done. https://chromiumcodereview.appspot.com/23702007/diff/61001/media/base/text_re... media/base/text_renderer.h:62: TextKind kind, On 2013/10/21 20:10:40, acolwell wrote: > nit: Use const TextTrackConfig& here. Done. https://chromiumcodereview.appspot.com/23702007/diff/61001/media/base/text_re... media/base/text_renderer.h:123: TextTrackMap text_track_map_; On 2013/10/21 20:10:40, acolwell wrote: > nit: Create a TextTrackState object that holds the ReadState & TextTrack so you > only have to maintain 1 map. Done. https://chromiumcodereview.appspot.com/23702007/diff/61001/media/base/text_tr... File media/base/text_track.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/61001/media/base/text_tr... media/base/text_track.cc:17: label_(label_), On 2013/10/21 20:10:40, acolwell wrote: > nit: s/_// in second label_ Done. https://chromiumcodereview.appspot.com/23702007/diff/61001/media/base/text_tr... media/base/text_track.cc:18: language_(language_) { On 2013/10/21 20:10:40, acolwell wrote: > ditto Done. https://chromiumcodereview.appspot.com/23702007/diff/61001/media/base/text_tr... media/base/text_track.cc:22: if (config.kind_ != kind_) On 2013/10/21 20:10:40, acolwell wrote: > nit: Merge all this into a single return statement. Done. https://chromiumcodereview.appspot.com/23702007/diff/61001/media/base/text_tr... File media/base/text_track.h (right): https://chromiumcodereview.appspot.com/23702007/diff/61001/media/base/text_tr... media/base/text_track.h:40: (TextKind kind, On 2013/10/21 20:10:40, acolwell wrote: > nit: Use const TextTrackConfig& here instead. Done. https://chromiumcodereview.appspot.com/23702007/diff/61001/media/base/text_tr... media/base/text_track.h:54: On 2013/10/21 20:10:40, acolwell wrote: > Add kind(), label(), language() accessors. Done. https://chromiumcodereview.appspot.com/23702007/diff/61001/media/filters/chun... File media/filters/chunk_demuxer.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/61001/media/filters/chun... media/filters/chunk_demuxer.cc:72: void TextSeek(TimeDelta); On 2013/10/21 20:10:40, acolwell wrote: > nit: Specify parameter name here. Done. https://chromiumcodereview.appspot.com/23702007/diff/61001/media/filters/chun... media/filters/chunk_demuxer.cc:274: STLDeleteValues(&text_stream_map_); On 2013/10/21 20:10:40, acolwell wrote: > I believe you may need to call Shutdown() on each of the streams to cancel any > pending reads they might have. Done. https://chromiumcodereview.appspot.com/23702007/diff/61001/media/filters/chun... media/filters/chunk_demuxer.cc:366: const TextTrackConfigMap& text_config) { On 2013/10/21 20:10:40, acolwell wrote: > nit: s/test_config/text_configs/ Done. https://chromiumcodereview.appspot.com/23702007/diff/61001/media/filters/chun... media/filters/chunk_demuxer.cc:436: const TextStreamMap::size_type text_count = text_stream_map_.size(); On 2013/10/21 20:10:40, acolwell wrote: > nit: Please just use size_t here. Done. https://chromiumcodereview.appspot.com/23702007/diff/61001/media/filters/chun... media/filters/chunk_demuxer.cc:448: // TODO(matthewjheaney): replace old with new On 2013/10/21 20:10:40, acolwell wrote: > I think this is just > > text_stream_map_.clear(). > text_stream_map_[config_itr->first] = text_stream; > > Right? Done. https://chromiumcodereview.appspot.com/23702007/diff/61001/media/filters/chun... media/filters/chunk_demuxer.cc:456: success &= false; On 2013/10/21 20:10:40, acolwell wrote: > nit: Break out of the loop here and then drop the else below & deindent. Done. https://chromiumcodereview.appspot.com/23702007/diff/61001/media/filters/chun... media/filters/chunk_demuxer.cc:467: // TODO(matthewjheaney): if all configs match, then replace old with new On 2013/10/21 20:10:40, acolwell wrote: > I don't think you need to do anything here. If they all match then everything > should be able to proceed as normal right? Done. https://chromiumcodereview.appspot.com/23702007/diff/61001/media/filters/chun... media/filters/chunk_demuxer.cc:755: if (!stream_) { On 2013/10/21 20:10:40, acolwell wrote: > Since we don't update TextTrackConfigs, you should be able to just > DCHECK(!stream_) here instead. Done. https://chromiumcodereview.appspot.com/23702007/diff/61001/media/filters/chun... media/filters/chunk_demuxer.cc:1055: // TODO(matthewjheaney): delete the text streams associated with this id On 2013/10/21 20:10:40, acolwell wrote: > nit: No longer needed since the streams get deleted with the SourceState. Done. https://chromiumcodereview.appspot.com/23702007/diff/61001/media/filters/chun... media/filters/chunk_demuxer.cc:1476: #if 0 On 2013/10/21 20:10:40, acolwell wrote: > remove this block if it is no longer needed. I restored the callback, so that the demuxer can notify the host. https://chromiumcodereview.appspot.com/23702007/diff/61001/media/filters/chun... File media/filters/chunk_demuxer.h (right): https://chromiumcodereview.appspot.com/23702007/diff/61001/media/filters/chun... media/filters/chunk_demuxer.h:175: TextKind kind, On 2013/10/21 20:10:40, acolwell wrote: > nit: Use const TextTrackConfig& here. Done. https://chromiumcodereview.appspot.com/23702007/diff/61001/media/filters/sour... File media/filters/source_buffer_stream.h (right): https://chromiumcodereview.appspot.com/23702007/diff/61001/media/filters/sour... media/filters/source_buffer_stream.h:320: // TODO(matthewjheaney): does this need to be a vector? On 2013/10/21 20:10:40, acolwell wrote: > no. since we don't allow config changes on text tracks right now. Done. https://chromiumcodereview.appspot.com/23702007/diff/61001/media/webm/webm_cl... File media/webm/webm_cluster_parser.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/61001/media/webm/webm_cl... media/webm/webm_cluster_parser.cc:359: std::vector<char> side_data; On 2013/10/21 20:10:40, acolwell wrote: > The side_data construction should be in a helper method so that the FFmpeg code > and this code can use the same logic. Done.
Getting closer. I'm pretty happy with the way this is turning out. :) Aside from the minor comment here, I think the TextTrack demuxer testing needs to be improved. They should be verifying that the calls on the DemuxerHost are actually happening at the appropriate times. TextRenderer also needs a unit test that verifies its behavior. https://codereview.chromium.org/23702007/diff/178001/content/renderer/media/t... File content/renderer/media/texttrack_impl.cc (right): https://codereview.chromium.org/23702007/diff/178001/content/renderer/media/t... content/renderer/media/texttrack_impl.cc:10: #include "media/base/bind_to_loop.h" nit:Remove. It doesn't appear to be used anywhere. https://codereview.chromium.org/23702007/diff/178001/media/base/decoder_buffer.h File media/base/decoder_buffer.h (right): https://codereview.chromium.org/23702007/diff/178001/media/base/decoder_buffe... media/base/decoder_buffer.h:139: static void MakeSideData(T id_begin, T id_end, This doesn't belong here. This class is format independent and this logic is WebVTT specific. I think placing this in something like media/filters/webvtt_util.h would be a better solution. https://codereview.chromium.org/23702007/diff/178001/media/base/pipeline.cc File media/base/pipeline.cc (right): https://codereview.chromium.org/23702007/diff/178001/media/base/pipeline.cc#n... media/base/pipeline.cc:306: #if 0 Remove #if https://codereview.chromium.org/23702007/diff/178001/media/base/pipeline.cc#n... media/base/pipeline.cc:957: if (text_renderer_) Why is this needed? It shouldn't be possible for this to get called after the pipeline has reached the kStopped state. The demuxers should not call Add/RemoveTextStream after they have called the callback passed to Stop(). https://codereview.chromium.org/23702007/diff/178001/media/base/pipeline.cc#n... media/base/pipeline.cc:965: demuxer_->Initialize(this, done_cb); WDYT about passing the enable_text_tracks bool here instead of through the demuxer constructors? You could use !text_renderer_.is_null() here and that would simplify the code in WebMediaPlayerImpl and prevent a situation where the pipeline and demuxers disagree about whether text tracks are enabled. https://codereview.chromium.org/23702007/diff/178001/media/base/stream_parser.h File media/base/stream_parser.h (right): https://codereview.chromium.org/23702007/diff/178001/media/base/stream_parser... media/base/stream_parser.h:72: const BufferQueue&)> NewTextBuffersCB; nit: I think this should fit on one line since the original code did. https://codereview.chromium.org/23702007/diff/178001/media/base/text_cue.h File media/base/text_cue.h (right): https://codereview.chromium.org/23702007/diff/178001/media/base/text_cue.h#ne... media/base/text_cue.h:35: virtual ~TextCue(); nit: I don't think this needs to be virtual since nothing derives from this object. https://codereview.chromium.org/23702007/diff/178001/media/base/text_renderer.cc File media/base/text_renderer.cc (right): https://codereview.chromium.org/23702007/diff/178001/media/base/text_renderer... media/base/text_renderer.cc:36: for (TextTrackStateMap::iterator itr = text_track_state_map_.begin(); nit: Use stl_util helper method here instead once the changes mentioned in the .h are done. https://codereview.chromium.org/23702007/diff/178001/media/base/text_renderer... media/base/text_renderer.cc:132: if (state_ == kStopPending || state_ == kStopped) I think these could be made into DCHECK_NEs. Since the demuxers are told to stop before the text renderer, you shouldn't receive any requests to add or remove text streams once the Stop() on the renderer has been called. If you do, that is a bug in the demuxer. https://codereview.chromium.org/23702007/diff/178001/media/base/text_renderer.h File media/base/text_renderer.h (right): https://codereview.chromium.org/23702007/diff/178001/media/base/text_renderer... media/base/text_renderer.h:58: // Add new |text_stream|, having the indicated |kind|, |label|, and nit: Update comment to refer to |config| instead. https://codereview.chromium.org/23702007/diff/178001/media/base/text_renderer... media/base/text_renderer.h:112: enum ReadState { nit: Move enum into TextTrackState. https://codereview.chromium.org/23702007/diff/178001/media/base/text_renderer... media/base/text_renderer.h:119: media::TextTrack* text_track; nit: You shouldn't need media:: here. make this a scoped_ptr<TextTrack> https://codereview.chromium.org/23702007/diff/178001/media/base/text_renderer... media/base/text_renderer.h:122: typedef std::map<DemuxerStream*, TextTrackState> TextTrackStateMap; nit: Make this TextTrackState* so the renderer can just delete the state object not have to worry about its internals. https://codereview.chromium.org/23702007/diff/178001/media/base/text_renderer... media/base/text_renderer.h:131: DISALLOW_IMPLICIT_CONSTRUCTORS(TextRenderer); nit: fix indent. https://codereview.chromium.org/23702007/diff/178001/media/base/text_track.h File media/base/text_track.h (right): https://codereview.chromium.org/23702007/diff/178001/media/base/text_track.h#... media/base/text_track.h:36: class TextTrackConfig { nit: Please put this class in its own file like the other XXConfig objects. https://codereview.chromium.org/23702007/diff/178001/media/filters/chunk_demu... File media/filters/chunk_demuxer.h (right): https://codereview.chromium.org/23702007/diff/178001/media/filters/chunk_demu... media/filters/chunk_demuxer.h:9: #include <set> nit: Remove? I don't think it is needed anymore. https://codereview.chromium.org/23702007/diff/178001/media/filters/fake_demux... File media/filters/fake_demuxer_stream.cc (right): https://codereview.chromium.org/23702007/diff/178001/media/filters/fake_demux... media/filters/fake_demuxer_stream.cc:15: #include "media/base/text_track.h" nit: Remove since it doesn't appear to be needed. https://codereview.chromium.org/23702007/diff/178001/media/filters/ffmpeg_dem... File media/filters/ffmpeg_demuxer_unittest.cc (right): https://codereview.chromium.org/23702007/diff/178001/media/filters/ffmpeg_dem... media/filters/ffmpeg_demuxer_unittest.cc:307: TEST_F(FFmpegDemuxerTest, Initialize_MultitrackText) { This test should also be verifying that the proper DemuxerHosts calls are made and that you are actually able to read data from the TextTrack DemuxerStreams. https://codereview.chromium.org/23702007/diff/178001/media/filters/source_buf... File media/filters/source_buffer_stream.cc (right): https://codereview.chromium.org/23702007/diff/178001/media/filters/source_buf... media/filters/source_buffer_stream.cc:371: text_track_config_ = text_config; nit: Please put this in the initializer list above. https://codereview.chromium.org/23702007/diff/178001/media/webm/webm_stream_p... File media/webm/webm_stream_parser.h (right): https://codereview.chromium.org/23702007/diff/178001/media/webm/webm_stream_p... media/webm/webm_stream_parser.h:30: bool enable_text_tracks, WDYT about just making text_cb null if text tracks aren't enabled? That would avoid the need for this extra parameter and also make it impossible for the parser to transmit text buffers if text tracks are disabled.
Is the title of this review misleading, or does it really mean that text rendering and whatnot will be done in ffmpeg and made part of the video frame? Why are the cues not just being sourced to a TextTrack and rendered in Blink just like out-of-band tracks?
On 2013/10/24 21:48:39, philipj wrote: > Is the title of this review misleading, or does it really mean that text > rendering and whatnot will be done in ffmpeg and made part of the video frame? > Why are the cues not just being sourced to a TextTrack and rendered in Blink > just like out-of-band tracks? Yes. The title is misleading. This CL does not have anything to do with rendering the actual cues. It is just providing a way for inband cues to get from the demuxers to Blink for rendering.
On 2013/10/24 21:48:39, philipj wrote: > Is the title of this review misleading, or does it really mean that text > rendering and whatnot will be done in ffmpeg and made part of the video frame? > Why are the cues not just being sourced to a TextTrack and rendered in Blink > just like out-of-band tracks? Yes. The title is misleading. This CL does not have anything to do with rendering the actual cues. It is just providing a way for inband cues to get from the demuxers to Blink for rendering.
Incorporated Aaron's comments. I had an outstanding question about the implementation of the mock demuxer host, but other than that everything else was handled. https://chromiumcodereview.appspot.com/23702007/diff/178001/content/renderer/... File content/renderer/media/texttrack_impl.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/178001/content/renderer/... content/renderer/media/texttrack_impl.cc:10: #include "media/base/bind_to_loop.h" On 2013/10/24 18:57:51, acolwell wrote: > nit:Remove. It doesn't appear to be used anywhere. Done. https://chromiumcodereview.appspot.com/23702007/diff/178001/media/base/decode... File media/base/decoder_buffer.h (right): https://chromiumcodereview.appspot.com/23702007/diff/178001/media/base/decode... media/base/decoder_buffer.h:139: static void MakeSideData(T id_begin, T id_end, On 2013/10/24 18:57:51, acolwell wrote: > This doesn't belong here. This class is format independent and this logic is > WebVTT specific. I think placing this in something like > media/filters/webvtt_util.h would be a better solution. Done. https://chromiumcodereview.appspot.com/23702007/diff/178001/media/base/pipeli... File media/base/pipeline.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/178001/media/base/pipeli... media/base/pipeline.cc:306: #if 0 On 2013/10/24 18:57:51, acolwell wrote: > Remove #if Done. https://chromiumcodereview.appspot.com/23702007/diff/178001/media/base/pipeli... media/base/pipeline.cc:957: if (text_renderer_) On 2013/10/24 18:57:51, acolwell wrote: > Why is this needed? It shouldn't be possible for this to get called after the > pipeline has reached the kStopped state. The demuxers should not call > Add/RemoveTextStream after they have called the callback passed to Stop(). Done. https://chromiumcodereview.appspot.com/23702007/diff/178001/media/base/stream... File media/base/stream_parser.h (right): https://chromiumcodereview.appspot.com/23702007/diff/178001/media/base/stream... media/base/stream_parser.h:72: const BufferQueue&)> NewTextBuffersCB; On 2013/10/24 18:57:51, acolwell wrote: > nit: I think this should fit on one line since the original code did. Done. https://chromiumcodereview.appspot.com/23702007/diff/178001/media/base/text_c... File media/base/text_cue.h (right): https://chromiumcodereview.appspot.com/23702007/diff/178001/media/base/text_c... media/base/text_cue.h:35: virtual ~TextCue(); On 2013/10/24 18:57:51, acolwell wrote: > nit: I don't think this needs to be virtual since nothing derives from this > object. Done. https://chromiumcodereview.appspot.com/23702007/diff/178001/media/base/text_r... File media/base/text_renderer.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/178001/media/base/text_r... media/base/text_renderer.cc:36: for (TextTrackStateMap::iterator itr = text_track_state_map_.begin(); On 2013/10/24 18:57:51, acolwell wrote: > nit: Use stl_util helper method here instead once the changes mentioned in the > .h are done. Done. https://chromiumcodereview.appspot.com/23702007/diff/178001/media/base/text_r... media/base/text_renderer.cc:132: if (state_ == kStopPending || state_ == kStopped) On 2013/10/24 18:57:51, acolwell wrote: > I think these could be made into DCHECK_NEs. Since the demuxers are told to stop > before the text renderer, you shouldn't receive any requests to add or remove > text streams once the Stop() on the renderer has been called. If you do, that is > a bug in the demuxer. Done. https://chromiumcodereview.appspot.com/23702007/diff/178001/media/base/text_r... File media/base/text_renderer.h (right): https://chromiumcodereview.appspot.com/23702007/diff/178001/media/base/text_r... media/base/text_renderer.h:58: // Add new |text_stream|, having the indicated |kind|, |label|, and On 2013/10/24 18:57:51, acolwell wrote: > nit: Update comment to refer to |config| instead. Done. https://chromiumcodereview.appspot.com/23702007/diff/178001/media/base/text_r... media/base/text_renderer.h:112: enum ReadState { On 2013/10/24 18:57:51, acolwell wrote: > nit: Move enum into TextTrackState. Done. https://chromiumcodereview.appspot.com/23702007/diff/178001/media/base/text_r... media/base/text_renderer.h:119: media::TextTrack* text_track; On 2013/10/24 18:57:51, acolwell wrote: > nit: You shouldn't need media:: here. > make this a scoped_ptr<TextTrack> Done. https://chromiumcodereview.appspot.com/23702007/diff/178001/media/base/text_r... media/base/text_renderer.h:122: typedef std::map<DemuxerStream*, TextTrackState> TextTrackStateMap; On 2013/10/24 18:57:51, acolwell wrote: > nit: Make this TextTrackState* so the renderer can just delete the state object > not have to worry about its internals. Done. https://chromiumcodereview.appspot.com/23702007/diff/178001/media/base/text_r... media/base/text_renderer.h:131: DISALLOW_IMPLICIT_CONSTRUCTORS(TextRenderer); On 2013/10/24 18:57:51, acolwell wrote: > nit: fix indent. Done. https://chromiumcodereview.appspot.com/23702007/diff/178001/media/base/text_t... File media/base/text_track.h (right): https://chromiumcodereview.appspot.com/23702007/diff/178001/media/base/text_t... media/base/text_track.h:36: class TextTrackConfig { On 2013/10/24 18:57:51, acolwell wrote: > nit: Please put this class in its own file like the other XXConfig objects. Done. https://chromiumcodereview.appspot.com/23702007/diff/178001/media/filters/chu... File media/filters/chunk_demuxer.h (right): https://chromiumcodereview.appspot.com/23702007/diff/178001/media/filters/chu... media/filters/chunk_demuxer.h:9: #include <set> On 2013/10/24 18:57:51, acolwell wrote: > nit: Remove? I don't think it is needed anymore. Done. https://chromiumcodereview.appspot.com/23702007/diff/178001/media/filters/fak... File media/filters/fake_demuxer_stream.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/178001/media/filters/fak... media/filters/fake_demuxer_stream.cc:15: #include "media/base/text_track.h" On 2013/10/24 18:57:51, acolwell wrote: > nit: Remove since it doesn't appear to be needed. Done. https://chromiumcodereview.appspot.com/23702007/diff/178001/media/filters/ffm... File media/filters/ffmpeg_demuxer_unittest.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/178001/media/filters/ffm... media/filters/ffmpeg_demuxer_unittest.cc:307: TEST_F(FFmpegDemuxerTest, Initialize_MultitrackText) { On 2013/10/24 18:57:51, acolwell wrote: > This test should also be verifying that the proper DemuxerHosts calls are made > and that you are actually able to read data from the TextTrack DemuxerStreams. Agreed, but we removed the text stream map member variable from the MockDemuxerHost, which was the intended mechanism for doing this. Did you have an alternative mechanism in mind? https://chromiumcodereview.appspot.com/23702007/diff/178001/media/filters/sou... File media/filters/source_buffer_stream.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/178001/media/filters/sou... media/filters/source_buffer_stream.cc:371: text_track_config_ = text_config; On 2013/10/24 18:57:51, acolwell wrote: > nit: Please put this in the initializer list above. Done. https://chromiumcodereview.appspot.com/23702007/diff/178001/media/webm/webm_s... File media/webm/webm_stream_parser.h (right): https://chromiumcodereview.appspot.com/23702007/diff/178001/media/webm/webm_s... media/webm/webm_stream_parser.h:30: bool enable_text_tracks, On 2013/10/24 18:57:51, acolwell wrote: > WDYT about just making text_cb null if text tracks aren't enabled? That would > avoid the need for this extra parameter and also make it impossible for the > parser to transmit text buffers if text tracks are disabled. Done.
https://chromiumcodereview.appspot.com/23702007/diff/178001/media/filters/ffm... File media/filters/ffmpeg_demuxer_unittest.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/178001/media/filters/ffm... media/filters/ffmpeg_demuxer_unittest.cc:307: TEST_F(FFmpegDemuxerTest, Initialize_MultitrackText) { On 2013/10/25 03:05:38, Matthew Heaney (Chromium) wrote: > On 2013/10/24 18:57:51, acolwell wrote: > > This test should also be verifying that the proper DemuxerHosts calls are made > > and that you are actually able to read data from the TextTrack DemuxerStreams. > > Agreed, but we removed the text stream map member variable from the > MockDemuxerHost, which was the intended mechanism for doing this. Did you have > an alternative mechanism in mind? Take a look at the gmock cookbook (https://code.google.com/p/googlemock/wiki/CookBook). You should be able to use EXPECT_CALL() w/ and SaveArg<0> to capture the stream objects and the call sequences.
On 2013/10/25 03:56:19, acolwell wrote: > https://chromiumcodereview.appspot.com/23702007/diff/178001/media/filters/ffm... > File media/filters/ffmpeg_demuxer_unittest.cc (right): > > https://chromiumcodereview.appspot.com/23702007/diff/178001/media/filters/ffm... > media/filters/ffmpeg_demuxer_unittest.cc:307: TEST_F(FFmpegDemuxerTest, > Initialize_MultitrackText) { > On 2013/10/25 03:05:38, Matthew Heaney (Chromium) wrote: > > On 2013/10/24 18:57:51, acolwell wrote: > > > This test should also be verifying that the proper DemuxerHosts calls are > made > > > and that you are actually able to read data from the TextTrack > DemuxerStreams. > > > > Agreed, but we removed the text stream map member variable from the > > MockDemuxerHost, which was the intended mechanism for doing this. Did you > have > > an alternative mechanism in mind? > Take a look at the gmock cookbook > (https://code.google.com/p/googlemock/wiki/CookBook). You should be able to use > EXPECT_CALL() w/ and SaveArg<0> to capture the stream objects and the call > sequences. OK, I added the SaveArg action to that ffmpeg demuxer unittest.
I pushed up another patch set (#9), with a small change to the ffmpeg demuxer unittest. https://chromiumcodereview.appspot.com/23702007/diff/178001/media/filters/ffm... File media/filters/ffmpeg_demuxer_unittest.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/178001/media/filters/ffm... media/filters/ffmpeg_demuxer_unittest.cc:307: TEST_F(FFmpegDemuxerTest, Initialize_MultitrackText) { On 2013/10/25 03:56:19, acolwell wrote: > On 2013/10/25 03:05:38, Matthew Heaney (Chromium) wrote: > > On 2013/10/24 18:57:51, acolwell wrote: > > > This test should also be verifying that the proper DemuxerHosts calls are > made > > > and that you are actually able to read data from the TextTrack > DemuxerStreams. > > > > Agreed, but we removed the text stream map member variable from the > > MockDemuxerHost, which was the intended mechanism for doing this. Did you > have > > an alternative mechanism in mind? > Take a look at the gmock cookbook > (https://code.google.com/p/googlemock/wiki/CookBook). You should be able to use > EXPECT_CALL() w/ and SaveArg<0> to capture the stream objects and the call > sequences. Done.
Just a few more minor nits. The main code looks good, but you need to significantly increase the amount of unit test coverage for all the new code paths you have added. At a minimum you need to cover the following: - FFmpegDemuxer & ChunkDemuxer behave properly when text tracks are present and text track support is enabled or disabled. This includes verifying that proper cue data is returned from the DemuxerStreams. - ChunkDemuxer properly handles tracks being added and removed as IDs are added and removed. - ChunkDemuxer properly rejects changes in the number and type of text tracks. - ChunkDemuxer properly handles text tracks in different IDs and only removes the ones for a specific ID. - Pipeline add text track, remove text track, and end of stream functionality needs unit tests. - TextRenderer needs unit tests that verify all aspects of its operation especially things like tracks coming and going and end of stream behavior when the tracks aren't the same length. - Any other tricky edge cases that you can come up with. In a separate CL you'll also want to start working on Blink LayoutTests that verify MSE & non-MSE inband text track behavior. This includes tests with seeking to make sure that duplicate cues are not created. https://chromiumcodereview.appspot.com/23702007/diff/178001/media/base/pipeli... File media/base/pipeline.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/178001/media/base/pipeli... media/base/pipeline.cc:965: demuxer_->Initialize(this, done_cb); On 2013/10/24 18:57:51, acolwell wrote: > WDYT about passing the enable_text_tracks bool here instead of through the > demuxer constructors? You could use !text_renderer_.is_null() here and that > would simplify the code in WebMediaPlayerImpl and prevent a situation where the > pipeline and demuxers disagree about whether text tracks are enabled. Please address this comment. https://chromiumcodereview.appspot.com/23702007/diff/688001/media/base/mock_d... File media/base/mock_demuxer_host.h (right): https://chromiumcodereview.appspot.com/23702007/diff/688001/media/base/mock_d... media/base/mock_demuxer_host.h:9: #include "media/base/text_track_config.h" nit: remove. I don't think you need this include. https://chromiumcodereview.appspot.com/23702007/diff/688001/media/base/stream... File media/base/stream_parser.h (right): https://chromiumcodereview.appspot.com/23702007/diff/688001/media/base/stream... media/base/stream_parser.h:26: typedef std::map<int, TextTrackConfig> TextTrackConfigMap; nit: place this inside the class definition w/ the other typedefs. https://chromiumcodereview.appspot.com/23702007/diff/688001/media/base/text_r... File media/base/text_renderer.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/688001/media/base/text_r... media/base/text_renderer.cc:67: DemuxerStream* const stream = itr->first; nit: fix indent here and the line below https://chromiumcodereview.appspot.com/23702007/diff/688001/media/base/text_r... media/base/text_renderer.cc:143: if (itr != text_track_state_map_.end()) { Should a DCHECK be used here instead? What is the situation where a caller would provide something that is not in the map? https://chromiumcodereview.appspot.com/23702007/diff/688001/media/base/text_r... media/base/text_renderer.cc:243: pause_cb_.Run(); I think you need base::ResetAndReturn(&pause_cb_) here. I also think that there might be a bug here if the last pending read happens to be an end of stream. If other streams haven't reached EOS yet then I think this code gets stuck. https://chromiumcodereview.appspot.com/23702007/diff/688001/media/base/text_r... media/base/text_renderer.cc:286: pause_cb_.Run(); Use base::ResetAndReturn(&pause_cb_) here. https://chromiumcodereview.appspot.com/23702007/diff/688001/media/base/text_r... media/base/text_renderer.cc:296: TextTrackState* state = new TextTrackState; nit: Use scoped_ptr here and release below. https://chromiumcodereview.appspot.com/23702007/diff/688001/media/base/text_r... media/base/text_renderer.cc:297: state->text_track.reset(text_track.release()); nit: This and setting read_state to kReadIdle should probably be in the TextTrackStaet constructor instead of done here. https://chromiumcodereview.appspot.com/23702007/diff/688001/media/base/text_r... media/base/text_renderer.cc:302: state->read_state = TextTrackState::kReadPending; nit: This appears to duplicate code in CueReady. Please put it in a helper method and have this and the CueReady code call the helper. https://chromiumcodereview.appspot.com/23702007/diff/688001/media/filters/web... File media/filters/webvtt_util.h (right): https://chromiumcodereview.appspot.com/23702007/diff/688001/media/filters/web... media/filters/webvtt_util.h:20: template<typename T> nit: Do we really need a template here? https://chromiumcodereview.appspot.com/23702007/diff/688001/media/media.gyp File media/media.gyp (right): https://chromiumcodereview.appspot.com/23702007/diff/688001/media/media.gyp#n... media/media.gyp:389: 'filters/wsola_internals.h', filters/webvtt_util.h is missing here. https://chromiumcodereview.appspot.com/23702007/diff/688001/media/mp2t/mp2t_s... File media/mp2t/mp2t_stream_parser_unittest.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/688001/media/mp2t/mp2t_s... media/mp2t/mp2t_stream_parser_unittest.cc:107: bool OnNewTextBuffers(int text_track, nit: Remove this since you are removing the reference below. https://chromiumcodereview.appspot.com/23702007/diff/688001/media/mp3/mp3_str... File media/mp3/mp3_stream_parser.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/688001/media/mp3/mp3_str... media/mp3/mp3_stream_parser.cc:121: const NewTextBuffersCB& /* text_cb */ , nit: Why does the name need to be in a comment now? It didn't before. https://chromiumcodereview.appspot.com/23702007/diff/688001/media/mp3/mp3_str... File media/mp3/mp3_stream_parser_unittest.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/688001/media/mp3/mp3_str... media/mp3/mp3_stream_parser_unittest.cc:84: bool OnNewTextBuffers(int text_track, nit: Remove since you are removing the reference below. https://chromiumcodereview.appspot.com/23702007/diff/688001/media/webm/webm_s... File media/webm/webm_stream_parser.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/688001/media/webm/webm_s... media/webm/webm_stream_parser.cc:210: TextTrackConfig(text_track_info.kind, It looks like TextTrackInfo is equivalent to TextTrackConfig. If you convert the code in WebMTracksParser you can eliminate this loop and just pass tracks_parser.text_tracks() to config_cb_.Run().
Incorporated most of Aaron's comments from most recent review. This patch set also contains a gtest harness for the text renderer class. I will include some or all of the modifications to the ffmpeg demuxer, chunk demuxer, and pipeline unit tests in the patch set that follows the review of this one. https://chromiumcodereview.appspot.com/23702007/diff/178001/media/base/pipeli... File media/base/pipeline.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/178001/media/base/pipeli... media/base/pipeline.cc:965: demuxer_->Initialize(this, done_cb); On 2013/10/24 18:57:51, acolwell wrote: > WDYT about passing the enable_text_tracks bool here instead of through the > demuxer constructors? You could use !text_renderer_.is_null() here and that > would simplify the code in WebMediaPlayerImpl and prevent a situation where the > pipeline and demuxers disagree about whether text tracks are enabled. Done. https://chromiumcodereview.appspot.com/23702007/diff/178001/media/base/pipeli... media/base/pipeline.cc:965: demuxer_->Initialize(this, done_cb); On 2013/10/29 21:14:19, acolwell wrote: > On 2013/10/24 18:57:51, acolwell wrote: > > WDYT about passing the enable_text_tracks bool here instead of through the > > demuxer constructors? You could use !text_renderer_.is_null() here and that > > would simplify the code in WebMediaPlayerImpl and prevent a situation where > the > > pipeline and demuxers disagree about whether text tracks are enabled. > > Please address this comment. Done. https://chromiumcodereview.appspot.com/23702007/diff/688001/media/base/mock_d... File media/base/mock_demuxer_host.h (right): https://chromiumcodereview.appspot.com/23702007/diff/688001/media/base/mock_d... media/base/mock_demuxer_host.h:9: #include "media/base/text_track_config.h" On 2013/10/29 21:14:19, acolwell wrote: > nit: remove. I don't think you need this include. gtest-printers.h complains about not having the full definition. https://chromiumcodereview.appspot.com/23702007/diff/688001/media/base/stream... File media/base/stream_parser.h (right): https://chromiumcodereview.appspot.com/23702007/diff/688001/media/base/stream... media/base/stream_parser.h:26: typedef std::map<int, TextTrackConfig> TextTrackConfigMap; On 2013/10/29 21:14:19, acolwell wrote: > nit: place this inside the class definition w/ the other typedefs. Done. https://chromiumcodereview.appspot.com/23702007/diff/688001/media/base/text_r... File media/base/text_renderer.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/688001/media/base/text_r... media/base/text_renderer.cc:67: DemuxerStream* const stream = itr->first; On 2013/10/29 21:14:19, acolwell wrote: > nit: fix indent here and the line below Done. https://chromiumcodereview.appspot.com/23702007/diff/688001/media/base/text_r... media/base/text_renderer.cc:143: if (itr != text_track_state_map_.end()) { On 2013/10/29 21:14:19, acolwell wrote: > Should a DCHECK be used here instead? What is the situation where a caller would > provide something that is not in the map? Done. https://chromiumcodereview.appspot.com/23702007/diff/688001/media/base/text_r... media/base/text_renderer.cc:243: pause_cb_.Run(); On 2013/10/29 21:14:19, acolwell wrote: > I think you need base::ResetAndReturn(&pause_cb_) here. > > I also think that there might be a bug here if the last pending read happens to > be an end of stream. If other streams haven't reached EOS yet then I think this > code gets stuck. Done. https://chromiumcodereview.appspot.com/23702007/diff/688001/media/base/text_r... media/base/text_renderer.cc:286: pause_cb_.Run(); On 2013/10/29 21:14:19, acolwell wrote: > Use base::ResetAndReturn(&pause_cb_) here. Done. https://chromiumcodereview.appspot.com/23702007/diff/688001/media/base/text_r... media/base/text_renderer.cc:296: TextTrackState* state = new TextTrackState; On 2013/10/29 21:14:19, acolwell wrote: > nit: Use scoped_ptr here and release below. Done. https://chromiumcodereview.appspot.com/23702007/diff/688001/media/base/text_r... media/base/text_renderer.cc:297: state->text_track.reset(text_track.release()); On 2013/10/29 21:14:19, acolwell wrote: > nit: This and setting read_state to kReadIdle should probably be in the > TextTrackStaet constructor instead of done here. Done. https://chromiumcodereview.appspot.com/23702007/diff/688001/media/base/text_r... media/base/text_renderer.cc:302: state->read_state = TextTrackState::kReadPending; On 2013/10/29 21:14:19, acolwell wrote: > nit: This appears to duplicate code in CueReady. Please put it in a helper > method and have this and the CueReady code call the helper. Done. https://chromiumcodereview.appspot.com/23702007/diff/688001/media/filters/web... File media/filters/webvtt_util.h (right): https://chromiumcodereview.appspot.com/23702007/diff/688001/media/filters/web... media/filters/webvtt_util.h:20: template<typename T> On 2013/10/29 21:14:19, acolwell wrote: > nit: Do we really need a template here? What is the alternative? In the ffmpeg demuxer case, you have uint8*, and in the chunk demuxer case, you have string. https://chromiumcodereview.appspot.com/23702007/diff/688001/media/media.gyp File media/media.gyp (right): https://chromiumcodereview.appspot.com/23702007/diff/688001/media/media.gyp#n... media/media.gyp:389: 'filters/wsola_internals.h', On 2013/10/29 21:14:19, acolwell wrote: > filters/webvtt_util.h is missing here. Done. https://chromiumcodereview.appspot.com/23702007/diff/688001/media/mp2t/mp2t_s... File media/mp2t/mp2t_stream_parser_unittest.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/688001/media/mp2t/mp2t_s... media/mp2t/mp2t_stream_parser_unittest.cc:107: bool OnNewTextBuffers(int text_track, On 2013/10/29 21:14:19, acolwell wrote: > nit: Remove this since you are removing the reference below. Done. https://chromiumcodereview.appspot.com/23702007/diff/688001/media/mp3/mp3_str... File media/mp3/mp3_stream_parser.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/688001/media/mp3/mp3_str... media/mp3/mp3_stream_parser.cc:121: const NewTextBuffersCB& /* text_cb */ , On 2013/10/29 21:14:19, acolwell wrote: > nit: Why does the name need to be in a comment now? It didn't before. Done. https://chromiumcodereview.appspot.com/23702007/diff/688001/media/mp3/mp3_str... File media/mp3/mp3_stream_parser_unittest.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/688001/media/mp3/mp3_str... media/mp3/mp3_stream_parser_unittest.cc:84: bool OnNewTextBuffers(int text_track, On 2013/10/29 21:14:19, acolwell wrote: > nit: Remove since you are removing the reference below. Done. https://chromiumcodereview.appspot.com/23702007/diff/688001/media/webm/webm_s... File media/webm/webm_stream_parser.cc (right): https://chromiumcodereview.appspot.com/23702007/diff/688001/media/webm/webm_s... media/webm/webm_stream_parser.cc:210: TextTrackConfig(text_track_info.kind, On 2013/10/29 21:14:19, acolwell wrote: > It looks like TextTrackInfo is equivalent to TextTrackConfig. If you convert the > code in WebMTracksParser you can eliminate this loop and just pass > tracks_parser.text_tracks() to config_cb_.Run(). Done.
https://codereview.chromium.org/23702007/diff/848001/media/base/text_renderer.cc File media/base/text_renderer.cc (right): https://codereview.chromium.org/23702007/diff/848001/media/base/text_renderer... media/base/text_renderer.cc:96: state_ = kPaused; Why is this transition needed now? I believe that Pipeline makes sure Pause() always precedes a Flush(). https://codereview.chromium.org/23702007/diff/848001/media/base/text_renderer... media/base/text_renderer.cc:295: if (pending_eos_set_.empty()) { I don't think we should call ended_cb_.Run() here since we are supposed to be paused at this point. IIUC ended_cb_ should get called when playback is resumed if no intervening Flush() occurs. https://codereview.chromium.org/23702007/diff/848001/media/base/text_renderer... media/base/text_renderer.cc:350: InsertResult result = text_track_state_map_.insert(value); wow. This does not make the code easier to read. How about just doing the following here text_track_state_map_[text_stream] = new TextTrackState(text_track.Pass()); and just use text_track_state_map_[text_stream] below. I think that would be way more straightforward that this and this code here isn't perf critical so I don't think the extra lookup is a big deal. https://codereview.chromium.org/23702007/diff/848001/media/base/text_renderer... media/base/text_renderer.cc:363: state->read_state = TextTrackState::kReadPending; nit: Add a DCHECK_NE(state->read_state, TextTrackState::kReadPending) to make sure you aren't already reading. https://codereview.chromium.org/23702007/diff/848001/media/base/text_renderer.h File media/base/text_renderer.h (right): https://codereview.chromium.org/23702007/diff/848001/media/base/text_renderer... media/base/text_renderer.h:88: struct TextTrackState; nit: Move the actual TextTrackState declaration right after private: above so you don't need to forward decl here. https://codereview.chromium.org/23702007/diff/848001/media/base/text_renderer... File media/base/text_renderer_unittest.cc (right): https://codereview.chromium.org/23702007/diff/848001/media/base/text_renderer... media/base/text_renderer_unittest.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. nit: s/ (c)// New files are supposed to use a header w/o the (c). https://codereview.chromium.org/23702007/diff/848001/media/base/text_renderer... media/base/text_renderer_unittest.cc:31: struct TextCueTest { This looks almost identical to the TextCue object. Use that instead. https://codereview.chromium.org/23702007/diff/848001/media/base/text_renderer... media/base/text_renderer_unittest.cc:59: class TextTrackTest : public TextTrack { nit:s/TextTrackTest/FakeTextTrack/ since this is actually a fake implementation and not a test for TextTrack. https://codereview.chromium.org/23702007/diff/848001/media/base/text_renderer... media/base/text_renderer_unittest.cc:69: RemoveTextTrackTest(text_renderer_test_, this); Pass a Closure into the constructor instead so this class doesn't need to know about the test and you don't have to do the forward decls or need the intermediary function. https://codereview.chromium.org/23702007/diff/848001/media/base/text_renderer... media/base/text_renderer_unittest.cc:157: scoped_refptr<base::MessageLoopProxy> message_loop_; Why do you need this? It doesn't look like you are using it in this class. https://codereview.chromium.org/23702007/diff/848001/media/base/text_renderer... media/base/text_renderer_unittest.cc:291: std::string content = "Girly Sound"; nit: I think we should have more neutral test text here and above. https://codereview.chromium.org/23702007/diff/848001/media/base/text_renderer... media/base/text_renderer_unittest.cc:299: TextTrackTest::Cues& cues = text_track->cues_; Do you really need to keep the cues around? It seems like you could just change addWebVTTCue() to a mock function and then just construct the appropriate EXPECT_CALL w/ all the expectations. https://codereview.chromium.org/23702007/diff/848001/media/base/text_renderer... media/base/text_renderer_unittest.cc:315: } To avoid duplicate code, it seems like SendCues() should be written in terms of SendCue(). https://codereview.chromium.org/23702007/diff/848001/media/base/text_renderer... media/base/text_renderer_unittest.cc:394: EXPECT_CALL(*text_stream, Read(_)) I think it might be better if you change DoRead() into Read() so you don't have to do these Invoke() calls everywhere. You could then just add a bool IsReadPending const { return !read_cb_.is_null(); } method to provide a way to check for pending reads when necessary. In many cases you may not even need this because the XXXPendingRead() helpers are already checking for a valid callback. I believe doing this will also allow you to merge CreateTextTrack() and AddTextTrack(). https://codereview.chromium.org/23702007/diff/848001/media/base/text_renderer... media/base/text_renderer_unittest.cc:399: message_loop_.RunUntilIdle(); nit: In almost all cases where Play() is called it is followed by a RunUntilIdle(). Please put this common sequences in a Play() helper function to help reduce the clutter. Do this for Pause and other cases where this also applies. https://codereview.chromium.org/23702007/diff/848001/media/base/text_renderer... media/base/text_renderer_unittest.cc:486: TEST_F(TextRendererTest, AddTrackAfterPlay2B) { nit: Typically when we have variants of the same test we use an _ & and descriptive name to differentiate the test case. For example AddTrackAfterPlay_FirstTrackAddedBeforePlay for this test and AddTrackAfterPlay_TwoTracksAddedAfterPlay. This makes it easier to understand what is being tested. Please fix this and any other similar instances below.
Fixed text renderer unit test per Aaron's suggestions. This patch set includes changes to the ffmpeg unit test and pipeline unit test. There are also some preliminary changes to the chunk demuxer unit test, but the changes to that unit test are not complete. https://codereview.chromium.org/23702007/diff/848001/media/base/text_renderer.cc File media/base/text_renderer.cc (right): https://codereview.chromium.org/23702007/diff/848001/media/base/text_renderer... media/base/text_renderer.cc:96: state_ = kPaused; On 2013/11/10 19:48:08, acolwell wrote: > Why is this transition needed now? I believe that Pipeline makes sure Pause() > always precedes a Flush(). OK, that's fine. I'll strengthen the precondition on Flush. https://codereview.chromium.org/23702007/diff/848001/media/base/text_renderer... media/base/text_renderer.cc:295: if (pending_eos_set_.empty()) { On 2013/11/10 19:48:08, acolwell wrote: > I don't think we should call ended_cb_.Run() here since we are supposed to be > paused at this point. IIUC ended_cb_ should get called when playback is resumed > if no intervening Flush() occurs. Done. https://codereview.chromium.org/23702007/diff/848001/media/base/text_renderer... media/base/text_renderer.cc:350: InsertResult result = text_track_state_map_.insert(value); On 2013/11/10 19:48:08, acolwell wrote: > wow. This does not make the code easier to read. How about just doing the > following here > text_track_state_map_[text_stream] = new TextTrackState(text_track.Pass()); > > and just use text_track_state_map_[text_stream] below. I think that would be way > more straightforward that this and this code here isn't perf critical so I don't > think the extra lookup is a big deal. I wanted a check there, but the compiler needed some help with type inference, hence the explicit types. Removed check and simplified. https://codereview.chromium.org/23702007/diff/848001/media/base/text_renderer... media/base/text_renderer.cc:363: state->read_state = TextTrackState::kReadPending; On 2013/11/10 19:48:08, acolwell wrote: > nit: Add a DCHECK_NE(state->read_state, TextTrackState::kReadPending) to make > sure you aren't already reading. Done. https://codereview.chromium.org/23702007/diff/848001/media/base/text_renderer.h File media/base/text_renderer.h (right): https://codereview.chromium.org/23702007/diff/848001/media/base/text_renderer... media/base/text_renderer.h:88: struct TextTrackState; On 2013/11/10 19:48:08, acolwell wrote: > nit: Move the actual TextTrackState declaration right after private: above so > you don't need to forward decl here. Done. https://codereview.chromium.org/23702007/diff/848001/media/base/text_renderer... File media/base/text_renderer_unittest.cc (right): https://codereview.chromium.org/23702007/diff/848001/media/base/text_renderer... media/base/text_renderer_unittest.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2013/11/10 19:48:08, acolwell wrote: > nit: s/ (c)// New files are supposed to use a header w/o the (c). Done. https://codereview.chromium.org/23702007/diff/848001/media/base/text_renderer... media/base/text_renderer_unittest.cc:31: struct TextCueTest { On 2013/11/10 19:48:08, acolwell wrote: > This looks almost identical to the TextCue object. Use that instead. Done. https://codereview.chromium.org/23702007/diff/848001/media/base/text_renderer... media/base/text_renderer_unittest.cc:59: class TextTrackTest : public TextTrack { On 2013/11/10 19:48:08, acolwell wrote: > nit:s/TextTrackTest/FakeTextTrack/ since this is actually a fake implementation > and not a test for TextTrack. Done. https://codereview.chromium.org/23702007/diff/848001/media/base/text_renderer... media/base/text_renderer_unittest.cc:69: RemoveTextTrackTest(text_renderer_test_, this); On 2013/11/10 19:48:08, acolwell wrote: > Pass a Closure into the constructor instead so this class doesn't need to know > about the test and you don't have to do the forward decls or need the > intermediary function. Done. https://codereview.chromium.org/23702007/diff/848001/media/base/text_renderer... media/base/text_renderer_unittest.cc:157: scoped_refptr<base::MessageLoopProxy> message_loop_; On 2013/11/10 19:48:08, acolwell wrote: > Why do you need this? It doesn't look like you are using it in this class. Done. https://codereview.chromium.org/23702007/diff/848001/media/base/text_renderer... media/base/text_renderer_unittest.cc:291: std::string content = "Girly Sound"; On 2013/11/10 19:48:08, acolwell wrote: > nit: I think we should have more neutral test text here and above. Done. https://codereview.chromium.org/23702007/diff/848001/media/base/text_renderer... media/base/text_renderer_unittest.cc:299: TextTrackTest::Cues& cues = text_track->cues_; On 2013/11/10 19:48:08, acolwell wrote: > Do you really need to keep the cues around? It seems like you could just change > addWebVTTCue() to a mock function and then just construct the appropriate > EXPECT_CALL w/ all the expectations. Done. https://codereview.chromium.org/23702007/diff/848001/media/base/text_renderer... media/base/text_renderer_unittest.cc:315: } On 2013/11/10 19:48:08, acolwell wrote: > To avoid duplicate code, it seems like SendCues() should be written in terms of > SendCue(). Done. https://codereview.chromium.org/23702007/diff/848001/media/base/text_renderer... media/base/text_renderer_unittest.cc:394: EXPECT_CALL(*text_stream, Read(_)) On 2013/11/10 19:48:08, acolwell wrote: > I think it might be better if you change DoRead() into Read() so you don't have > to do these Invoke() calls everywhere. You could then just add a bool > IsReadPending const { return !read_cb_.is_null(); } method to provide a way to > check for pending reads when necessary. In many cases you may not even need this > because the XXXPendingRead() helpers are already checking for a valid callback. > > I believe doing this will also allow you to merge CreateTextTrack() and > AddTextTrack(). Done. https://codereview.chromium.org/23702007/diff/848001/media/base/text_renderer... media/base/text_renderer_unittest.cc:399: message_loop_.RunUntilIdle(); On 2013/11/10 19:48:08, acolwell wrote: > nit: In almost all cases where Play() is called it is followed by a > RunUntilIdle(). Please put this common sequences in a Play() helper function to > help reduce the clutter. Do this for Pause and other cases where this also > applies. Done. https://codereview.chromium.org/23702007/diff/848001/media/base/text_renderer... media/base/text_renderer_unittest.cc:486: TEST_F(TextRendererTest, AddTrackAfterPlay2B) { On 2013/11/10 19:48:08, acolwell wrote: > nit: Typically when we have variants of the same test we use an _ & and > descriptive name to differentiate the test case. For example > AddTrackAfterPlay_FirstTrackAddedBeforePlay for this test and > AddTrackAfterPlay_TwoTracksAddedAfterPlay. This makes it easier to understand > what is being tested. Please fix this and any other similar instances below. Done.
https://codereview.chromium.org/74623002/ suggests creating more work for this review.
So, I've been seeing the APIs for in-band tracks in Blink a bit lately, and have a question for you. It seems to me that the WebInbandTextTrack object doesn't really serve any purpose, since its members are only ever use once, and could just as well be the arguments to the addTextTrack callback on WebMediaPlayerClient. Instead of an object, the track could simply be given a unique ID on the platform side, that is used when adding the track, sourcing cues, and when removing it. I haven't looked so closely at the implementation in this review, but my guess is that this ought to simplify things a bit and require allocating and keeping track of fewer objects. Yes, no?
On 2013/11/16 16:00:50, philipj wrote: > So, I've been seeing the APIs for in-band tracks in Blink a bit lately, and have > a question for you. It seems to me that the WebInbandTextTrack object doesn't > really serve any purpose, since its members are only ever use once, and could > just as well be the arguments to the addTextTrack callback on > WebMediaPlayerClient. Instead of an object, the track could simply be given a > unique ID on the platform side, that is used when adding the track, sourcing > cues, and when removing it. My understanding (Aaron and Andrew can elaborate) is that this is a stylized interface between Chrome and Blink. Note too that WebInbandTextTrack methods run on the main (render) loop. On change must satisfy the constraint that some work needs to be done on separate threads. > I haven't looked so closely at the implementation in this review, but my guess > is that this ought to simplify things a bit and require allocating and keeping > track of fewer objects. Yes, no? The WebInbandTextTrackImpl object is owned by the TextTrackImpl object, which also runs on the main loop. There has to be some why for Chrome to push text track cues to the HTML rendering engine. Right now the new text renderer object calls the addWebVTTCue method of the TextTrackImpl object, which forwards it to the WebMediaPlayerClient, and that's Chrome's hook into the HTML rendering stack. The only object that needs to keep track of anything is the text renderer object. It runs on the media loop, so some callbacks need to be run on the main loop in order to add and remove text tracks.
On 2013/11/16 16:00:50, philipj wrote: > So, I've been seeing the APIs for in-band tracks in Blink a bit lately, and have > a question for you. It seems to me that the WebInbandTextTrack object doesn't > really serve any purpose, since its members are only ever use once, and could > just as well be the arguments to the addTextTrack callback on > WebMediaPlayerClient. Instead of an object, the track could simply be given a > unique ID on the platform side, that is used when adding the track, sourcing > cues, and when removing it. My understanding (Aaron and Andrew can elaborate) is that this is a stylized interface between Chrome and Blink. Note too that WebInbandTextTrack methods run on the main (render) loop. On change must satisfy the constraint that some work needs to be done on separate threads. > I haven't looked so closely at the implementation in this review, but my guess > is that this ought to simplify things a bit and require allocating and keeping > track of fewer objects. Yes, no? The WebInbandTextTrackImpl object is owned by the TextTrackImpl object, which also runs on the main loop. There has to be some why for Chrome to push text track cues to the HTML rendering engine. Right now the new text renderer object calls the addWebVTTCue method of the TextTrackImpl object, which forwards it to the WebMediaPlayerClient, and that's Chrome's hook into the HTML rendering stack. The only object that needs to keep track of anything is the text renderer object. It runs on the media loop, so some callbacks need to be run on the main loop in order to add and remove text tracks.
lgtm % comments My assumption is that you file bugs for any other outstanding issues you are aware of and will provide followup CLs for them. https://codereview.chromium.org/23702007/diff/1318001/media/base/pipeline.cc File media/base/pipeline.cc (right): https://codereview.chromium.org/23702007/diff/1318001/media/base/pipeline.cc#... media/base/pipeline.cc:947: text_renderer_->AddTextStream(text_stream, config); File a bug to update text_ended_ state when new streams are added. If all the existing text streams have ended, they would cause text_ended_ to become true, but if a new stream is added before the audio_ended_ & video_ended_ become true, then we need to clear that state so we wait for the newly added track to end. https://codereview.chromium.org/23702007/diff/1318001/media/base/text_rendere... File media/base/text_renderer.cc (right): https://codereview.chromium.org/23702007/diff/1318001/media/base/text_rendere... media/base/text_renderer.cc:193: stop_cb_.Run(); This should be base::ResetAndReturn(&stop_cb_).Run(). https://codereview.chromium.org/23702007/diff/1318001/media/base/text_rendere... media/base/text_renderer.cc:298: stop_cb_.Run(); base::ResetAndReturn(&stop_cb_).Run(); https://codereview.chromium.org/23702007/diff/1318001/media/base/text_rendere... File media/base/text_renderer_unittest.cc (right): https://codereview.chromium.org/23702007/diff/1318001/media/base/text_rendere... media/base/text_renderer_unittest.cc:29: class FakeTextTrack; nit: I don't think these 2 forward declarations are needed anymore. https://codereview.chromium.org/23702007/diff/1318001/media/base/text_rendere... media/base/text_renderer_unittest.cc:58: class TextTrackStream : public DemuxerStream { Why do you need this? This looks very similar to FakeTextTrackStream. Please rework these tests in terms of FakeTextTrackStream so we don't have duplicate code. https://codereview.chromium.org/23702007/diff/1318001/media/base/text_rendere... media/base/text_renderer_unittest.cc:238: EXPECT_CALL(*text_track, addWebVTTCue(Eq(start), nit: You shouldn't need the Eq on all the parameters. Is the compiler complaining if you leave them off? https://codereview.chromium.org/23702007/diff/1318001/media/base/text_rendere... media/base/text_renderer_unittest.cc:243: .Times(Exactly(1)); nit: I don't think you should need the Times(Exactly(1)). I believe that is the default for EXPECT_CALL(). https://codereview.chromium.org/23702007/diff/1318001/media/base/text_rendere... media/base/text_renderer_unittest.cc:251: for (unsigned idx = 0; idx < text_track_streams_.size(); ++idx) { nit: You should use size_t here and in other similar contexts in this file since that is the actual type size() returns. https://codereview.chromium.org/23702007/diff/1318001/media/base/text_rendere... media/base/text_renderer_unittest.cc:287: EXPECT_CALL(*stream, OnRead()).Times(1); nit: Times(1) should be unnecessary. https://codereview.chromium.org/23702007/diff/1318001/media/filters/chunk_dem... File media/filters/chunk_demuxer.cc (right): https://codereview.chromium.org/23702007/diff/1318001/media/filters/chunk_dem... media/filters/chunk_demuxer.cc:455: success &= false; nit: Add MEDIA_LOG(log_cb_) << "The number of text track configs changed." https://codereview.chromium.org/23702007/diff/1318001/media/filters/chunk_dem... media/filters/chunk_demuxer.cc:463: success &= false; nit: Add MEDIA_LOG(log_cb_) << "New text track config does not match the old one." https://codereview.chromium.org/23702007/diff/1318001/media/filters/chunk_dem... media/filters/chunk_demuxer.cc:474: success &= false; nit: Add MEDIA_LOG(log_cb_) << "Unexpected text track configuration for track ID " << config_itr->first; https://codereview.chromium.org/23702007/diff/1318001/media/filters/chunk_dem... media/filters/chunk_demuxer.cc:482: success &= false; nit: Add MEDIA_LOG(log_cb_) << "New text track config for track ID " << config_itr->first << " does not match the old one." https://codereview.chromium.org/23702007/diff/1318001/media/filters/chunk_dem... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/23702007/diff/1318001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:207: // TODO(matthewjheaney): create an abstraction to do this. File a bug to track this and add the crbug url here. https://codereview.chromium.org/23702007/diff/1318001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:1698: TEST_F(ChunkDemuxerTest, AddSeparateSourcesForAudioAndVideoText) { File a bug to track adding more ChunkDemuxer tests for text tracks. https://codereview.chromium.org/23702007/diff/1318001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer_unittest.cc (right): https://codereview.chromium.org/23702007/diff/1318001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer_unittest.cc:431: const int kMaxBuffers = 10000; nit: Please trim this down to a smaller number. I'm assuming the number of cues in the file is much smaller than 10000. Reading 1 cue beyond the actual number should trigger failure. https://codereview.chromium.org/23702007/diff/1318001/media/filters/webvtt_ut... File media/filters/webvtt_util.h (right): https://codereview.chromium.org/23702007/diff/1318001/media/filters/webvtt_ut... media/filters/webvtt_util.h:21: inline void media::MakeSideData( nit: The implementation should be inside the namespace. Why is it here? If there is a compiler issue, try removing the "static" in the declaration above and see if this allows you to put everything inside the namespace.
Integrated Aaron's suggestions. https://codereview.chromium.org/23702007/diff/1318001/media/base/pipeline.cc File media/base/pipeline.cc (right): https://codereview.chromium.org/23702007/diff/1318001/media/base/pipeline.cc#... media/base/pipeline.cc:947: text_renderer_->AddTextStream(text_stream, config); On 2013/11/20 01:15:24, acolwell wrote: > File a bug to update text_ended_ state when new streams are added. > If all the existing text streams have ended, they would cause text_ended_ to > become true, but if a new stream is added before the audio_ended_ & video_ended_ > become true, then we need to clear that state so we wait for the newly added > track to end. Created CR: http://crbug.com/321446 https://codereview.chromium.org/23702007/diff/1318001/media/base/text_rendere... File media/base/text_renderer.cc (right): https://codereview.chromium.org/23702007/diff/1318001/media/base/text_rendere... media/base/text_renderer.cc:193: stop_cb_.Run(); On 2013/11/20 01:15:24, acolwell wrote: > This should be base::ResetAndReturn(&stop_cb_).Run(). Done. https://codereview.chromium.org/23702007/diff/1318001/media/base/text_rendere... media/base/text_renderer.cc:298: stop_cb_.Run(); On 2013/11/20 01:15:24, acolwell wrote: > base::ResetAndReturn(&stop_cb_).Run(); Done. https://codereview.chromium.org/23702007/diff/1318001/media/base/text_rendere... File media/base/text_renderer_unittest.cc (right): https://codereview.chromium.org/23702007/diff/1318001/media/base/text_rendere... media/base/text_renderer_unittest.cc:29: class FakeTextTrack; On 2013/11/20 01:15:24, acolwell wrote: > nit: I don't think these 2 forward declarations are needed anymore. Done. https://codereview.chromium.org/23702007/diff/1318001/media/base/text_rendere... media/base/text_renderer_unittest.cc:58: class TextTrackStream : public DemuxerStream { On 2013/11/20 01:15:24, acolwell wrote: > Why do you need this? This looks very similar to FakeTextTrackStream. Please > rework these tests in terms of FakeTextTrackStream so we don't have duplicate > code. Done. https://codereview.chromium.org/23702007/diff/1318001/media/base/text_rendere... media/base/text_renderer_unittest.cc:238: EXPECT_CALL(*text_track, addWebVTTCue(Eq(start), On 2013/11/20 01:15:24, acolwell wrote: > nit: You shouldn't need the Eq on all the parameters. Is the compiler > complaining if you leave them off? Done. https://codereview.chromium.org/23702007/diff/1318001/media/base/text_rendere... media/base/text_renderer_unittest.cc:243: .Times(Exactly(1)); On 2013/11/20 01:15:24, acolwell wrote: > nit: I don't think you should need the Times(Exactly(1)). I believe that is the > default for EXPECT_CALL(). Done. https://codereview.chromium.org/23702007/diff/1318001/media/base/text_rendere... media/base/text_renderer_unittest.cc:251: for (unsigned idx = 0; idx < text_track_streams_.size(); ++idx) { On 2013/11/20 01:15:24, acolwell wrote: > nit: You should use size_t here and in other similar contexts in this file since > that is the actual type size() returns. Done. https://codereview.chromium.org/23702007/diff/1318001/media/base/text_rendere... media/base/text_renderer_unittest.cc:287: EXPECT_CALL(*stream, OnRead()).Times(1); On 2013/11/20 01:15:24, acolwell wrote: > nit: Times(1) should be unnecessary. Done. https://codereview.chromium.org/23702007/diff/1318001/media/filters/chunk_dem... File media/filters/chunk_demuxer.cc (right): https://codereview.chromium.org/23702007/diff/1318001/media/filters/chunk_dem... media/filters/chunk_demuxer.cc:455: success &= false; On 2013/11/20 01:15:24, acolwell wrote: > nit: Add MEDIA_LOG(log_cb_) << "The number of text track configs changed." Done. https://codereview.chromium.org/23702007/diff/1318001/media/filters/chunk_dem... media/filters/chunk_demuxer.cc:463: success &= false; On 2013/11/20 01:15:24, acolwell wrote: > nit: Add MEDIA_LOG(log_cb_) << "New text track config does not match the old > one." Done. https://codereview.chromium.org/23702007/diff/1318001/media/filters/chunk_dem... media/filters/chunk_demuxer.cc:474: success &= false; On 2013/11/20 01:15:24, acolwell wrote: > nit: Add MEDIA_LOG(log_cb_) << "Unexpected text track configuration for track ID > " << config_itr->first; Done. https://codereview.chromium.org/23702007/diff/1318001/media/filters/chunk_dem... media/filters/chunk_demuxer.cc:482: success &= false; On 2013/11/20 01:15:24, acolwell wrote: > nit: Add MEDIA_LOG(log_cb_) << "New text track config for track ID " << > config_itr->first << " does not match the old one." Done. https://codereview.chromium.org/23702007/diff/1318001/media/filters/chunk_dem... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/23702007/diff/1318001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:207: // TODO(matthewjheaney): create an abstraction to do this. On 2013/11/20 01:15:24, acolwell wrote: > File a bug to track this and add the crbug url here. Created CR: https://code.google.com/p/chromium/issues/detail?id=321454 https://codereview.chromium.org/23702007/diff/1318001/media/filters/chunk_dem... media/filters/chunk_demuxer_unittest.cc:1698: TEST_F(ChunkDemuxerTest, AddSeparateSourcesForAudioAndVideoText) { On 2013/11/20 01:15:24, acolwell wrote: > File a bug to track adding more ChunkDemuxer tests for text tracks. Added CR: https://code.google.com/p/chromium/issues/detail?id=321455 https://codereview.chromium.org/23702007/diff/1318001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer_unittest.cc (right): https://codereview.chromium.org/23702007/diff/1318001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer_unittest.cc:431: const int kMaxBuffers = 10000; On 2013/11/20 01:15:24, acolwell wrote: > nit: Please trim this down to a smaller number. I'm assuming the number of cues > in the file is much smaller than 10000. Reading 1 cue beyond the actual number > should trigger failure. Done. https://codereview.chromium.org/23702007/diff/1318001/media/filters/webvtt_ut... File media/filters/webvtt_util.h (right): https://codereview.chromium.org/23702007/diff/1318001/media/filters/webvtt_ut... media/filters/webvtt_util.h:21: inline void media::MakeSideData( On 2013/11/20 01:15:24, acolwell wrote: > nit: The implementation should be inside the namespace. Why is it here? If there > is a compiler issue, try removing the "static" in the declaration above and see > if this allows you to put everything inside the namespace. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/matthewjheaney@chromium.org/23702007/1...
Retried try job too often on mac_rel for step(s) base_unittests, browser_tests, interactive_ui_tests, net_unittests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/matthewjheaney@chromium.org/23702007/1...
Retried try job too often on linux_clang for step(s) compile http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/matthewjheaney@chromium.org/23702007/1...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/matthewjheaney@chromium.org/23702007/1...
Failed to trigger a try job on win_x64_rel HTTP Error 400: Bad Request
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/matthewjheaney@chromium.org/23702007/2...
https://codereview.chromium.org/23702007/diff/2148001/content/renderer/media/... File content/renderer/media/android/media_source_delegate.cc (right): https://codereview.chromium.org/23702007/diff/2148001/content/renderer/media/... content/renderer/media/android/media_source_delegate.cc:511: // TODO(matthewjheaney): do something here Put NOTIMPLEMENTED(); here and below. File a bug for adding inband text track rendering support for Android and include the link here.
Retried try job too often on android_clang_dbg for step(s) slave_steps http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Added CR note. https://codereview.chromium.org/23702007/diff/2148001/content/renderer/media/... File content/renderer/media/android/media_source_delegate.cc (right): https://codereview.chromium.org/23702007/diff/2148001/content/renderer/media/... content/renderer/media/android/media_source_delegate.cc:511: // TODO(matthewjheaney): do something here On 2013/11/21 02:49:18, acolwell wrote: > Put NOTIMPLEMENTED(); here and below. File a bug for adding inband text track > rendering support for Android and include the link here. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/matthewjheaney@chromium.org/23702007/2...
Retried try job too often on android_clang_dbg for step(s) slave_steps http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/matthewjheaney@chromium.org/23702007/1...
Retried try job too often on win_x64_rel for step(s) base_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/matthewjheaney@chromium.org/23702007/2...
Message was sent while issue was closed.
Change committed as 236660 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
