Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
151103005:
TextRenderer only pushes new cues downstream (Closed)
|
Created:
6 years, 10 months ago by Matthew Heaney (Chromium) Modified:
6 years, 10 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionTextRenderer only pushes new cues downstream
The TextTrack subsystem in Blink unconditionally satisfies a request
to add a new text cue to the text track cue list, relying on the
upstream demuxers to filter out cues that have already been added to
that list.
The TextRenderer now manages a map of time range objects, to keep
track of which cues (based on their start times) have already been
pushed downstream prior to the current seek.
BUG=230708
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=249845
Patch Set 1 #Patch Set 2 : added text ranges utility and unit test #
Total comments: 28
Patch Set 3 : incorporate aaron's comments of 2014/02/05 #
Total comments: 16
Patch Set 4 : incorporate aaron's comments 2014/02/06 #
Total comments: 10
Patch Set 5 : incorporate aaron's comments of 2014/02/07 #Patch Set 6 : fix style errors #Patch Set 7 : fix signed/unsigned int compare #
Messages
Total messages: 21 (0 generated)
I modified the TextRenderer to keep track of text cues, using Aaron's suggested implementation approach. For now the algorithm lives (only) in the TextRenderer, for the purposes of review. Assuming this is close to what Aaron had in mind, we can refactor the code as necessary.
This looks reasonable after a quick skim. Please put the range tracking logic in a helper class like I originally proposed and provide unit tests for that class so that it is easy to verify the filtering logic w/o having to deal with all the TextRenderer machinery.
Factored out text ranges functionality into its own utility module, and added associated unit test.
https://codereview.chromium.org/151103005/diff/50001/media/base/text_ranges.cc File media/base/text_ranges.cc (right): https://codereview.chromium.org/151103005/diff/50001/media/base/text_ranges.c... media/base/text_ranges.cc:20: typedef RangeMap::iterator Itr; nit: Please just use the full type below. https://codereview.chromium.org/151103005/diff/50001/media/base/text_ranges.c... media/base/text_ranges.cc:24: // follows a ssek. nit:s/ssek/Reset()/ https://codereview.chromium.org/151103005/diff/50001/media/base/text_ranges.c... media/base/text_ranges.cc:31: const Itr first_itr = range_map_.begin(); nit: Inline this since it is only used on the next line. https://codereview.chromium.org/151103005/diff/50001/media/base/text_ranges.c... media/base/text_ranges.cc:61: if (start_time < curr_range.last_time) { Please put the logic on lines 61 - 74 in a method on Range. The multiple references to curr_range imply that this really should be a method. https://codereview.chromium.org/151103005/diff/50001/media/base/text_ranges.c... media/base/text_ranges.cc:90: next_range.count = 0; nit: This logic is duplicated. It looks like a ResetCount(start_time) should be added to Range https://codereview.chromium.org/151103005/diff/50001/media/base/text_ranges.c... media/base/text_ranges.cc:94: curr_range = next_range; nit: I think hiding the logic in this block in a void Merge(const Range&) method on Range would make this cleaner and remove the need for the next_range temporary. https://codereview.chromium.org/151103005/diff/50001/media/base/text_ranges.c... media/base/text_ranges.cc:108: curr_range.last_time = start_time; nit: Place these assignments in an SetLastTime() method on Range. https://codereview.chromium.org/151103005/diff/50001/media/base/text_ranges.c... media/base/text_ranges.cc:117: range.last_time = start_time; nit: Use the SetLastTime() method mentioned above here to avoid duplication. https://codereview.chromium.org/151103005/diff/50001/media/base/text_ranges.c... media/base/text_ranges.cc:121: typedef RangeMap::iterator Itr; nit: Please remove the typdefs and inline since this is only used once and the inline is more compact. https://codereview.chromium.org/151103005/diff/50001/media/base/text_ranges.h File media/base/text_ranges.h (right): https://codereview.chromium.org/151103005/diff/50001/media/base/text_ranges.h... media/base/text_ranges.h:22: void Flush(); nit: The name of the method and the comment above it don't seem to match up. Usually words in the method name appear in the comment describing its behavior. Perhaps the method should be named something like Reset() so that the comment could be something along the lines of "Reset the current range pointer so that this object will bind to a new current range on the next AddCue()." https://codereview.chromium.org/151103005/diff/50001/media/base/text_ranges.h... media/base/text_ranges.h:24: // To test whether a cue has already been pushed downstream. nit: This does more than simply test whether the cue has been pushed downstream. There should be more context here indicating that start_time is used to update the range map and the return value should be documanted. If this simply tested whether a cue has already been pushed, I'd expect true to be returned when the cue had been already pushed, but this method would return false in that case . There should also be text about the expectations around start_time and successive AddCue() calls. For example, start_time is expected to be monotonically increasing across calls unless there is an intervening Reset(). https://codereview.chromium.org/151103005/diff/50001/media/base/text_ranges.h... media/base/text_ranges.h:50: // matches the seek time. nit: Technically this isn't a seek time. It's the start time of the first cue after a seek. You should reword this in terms of Reset() instead of seek since seeking is really outside the scope of this helper class. https://codereview.chromium.org/151103005/diff/50001/media/base/text_ranges.h... media/base/text_ranges.h:58: // The time range to which we bind following a seek. nit: s/seek/Reset()/ https://codereview.chromium.org/151103005/diff/50001/media/base/text_ranges_u... File media/base/text_ranges_unittest.cc (right): https://codereview.chromium.org/151103005/diff/50001/media/base/text_ranges_u... media/base/text_ranges_unittest.cc:18: EXPECT_EQ(r.Present(base::TimeDelta::FromSeconds(5)), 1); Please remove the Present() calls and Present() itself. They just clutter the tests. You shouldn't need them since production code doesn't access them and you should be able to verify proper behavior solely with combinations of addCue() and Reset() calls. The one exception I can think of is if you want to make sure that range merging is happening properly. In that case having a simple size_t RangeCountForTesting() const method that returns the number of ranges should be sufficient. You should definitely use this sparingly though and not include the verification in every test. Just in a few key focused ones.
Incorporated Aaron's comments (2014/02/05, Wed). https://codereview.chromium.org/151103005/diff/50001/media/base/text_ranges.cc File media/base/text_ranges.cc (right): https://codereview.chromium.org/151103005/diff/50001/media/base/text_ranges.c... media/base/text_ranges.cc:20: typedef RangeMap::iterator Itr; On 2014/02/05 19:09:36, acolwell wrote: > nit: Please just use the full type below. ack https://codereview.chromium.org/151103005/diff/50001/media/base/text_ranges.c... media/base/text_ranges.cc:24: // follows a ssek. On 2014/02/05 19:09:36, acolwell wrote: > nit:s/ssek/Reset()/ Done. https://codereview.chromium.org/151103005/diff/50001/media/base/text_ranges.c... media/base/text_ranges.cc:31: const Itr first_itr = range_map_.begin(); On 2014/02/05 19:09:36, acolwell wrote: > nit: Inline this since it is only used on the next line. Done. https://codereview.chromium.org/151103005/diff/50001/media/base/text_ranges.c... media/base/text_ranges.cc:61: if (start_time < curr_range.last_time) { On 2014/02/05 19:09:36, acolwell wrote: > Please put the logic on lines 61 - 74 in a method on Range. The multiple > references to curr_range imply that this really should be a method. Done. https://codereview.chromium.org/151103005/diff/50001/media/base/text_ranges.c... media/base/text_ranges.cc:90: next_range.count = 0; On 2014/02/05 19:09:36, acolwell wrote: > nit: This logic is duplicated. It looks like a ResetCount(start_time) should be > added to Range Done. https://codereview.chromium.org/151103005/diff/50001/media/base/text_ranges.c... media/base/text_ranges.cc:94: curr_range = next_range; On 2014/02/05 19:09:36, acolwell wrote: > nit: I think hiding the logic in this block in a void Merge(const Range&) method > on Range would make this cleaner and remove the need for the next_range > temporary. Done. https://codereview.chromium.org/151103005/diff/50001/media/base/text_ranges.c... media/base/text_ranges.cc:108: curr_range.last_time = start_time; On 2014/02/05 19:09:36, acolwell wrote: > nit: Place these assignments in an SetLastTime() method on Range. Done. https://codereview.chromium.org/151103005/diff/50001/media/base/text_ranges.c... media/base/text_ranges.cc:117: range.last_time = start_time; On 2014/02/05 19:09:36, acolwell wrote: > nit: Use the SetLastTime() method mentioned above here to avoid duplication. Done. https://codereview.chromium.org/151103005/diff/50001/media/base/text_ranges.c... media/base/text_ranges.cc:121: typedef RangeMap::iterator Itr; On 2014/02/05 19:09:36, acolwell wrote: > nit: Please remove the typdefs and inline since this is only used once and the > inline is more compact. Done. https://codereview.chromium.org/151103005/diff/50001/media/base/text_ranges.h File media/base/text_ranges.h (right): https://codereview.chromium.org/151103005/diff/50001/media/base/text_ranges.h... media/base/text_ranges.h:22: void Flush(); On 2014/02/05 19:09:36, acolwell wrote: > nit: The name of the method and the comment above it don't seem to match up. > Usually words in the method name appear in the comment describing its behavior. > Perhaps the method should be named something like Reset() so that the comment > could be something along the lines of "Reset the current range pointer so that > this object will bind to a new current range on the next AddCue()." Done. https://codereview.chromium.org/151103005/diff/50001/media/base/text_ranges.h... media/base/text_ranges.h:24: // To test whether a cue has already been pushed downstream. On 2014/02/05 19:09:36, acolwell wrote: > nit: This does more than simply test whether the cue has been pushed downstream. > There should be more context here indicating that start_time is used to update > the range map and the return value should be documanted. If this simply tested > whether a cue has already been pushed, I'd expect true to be returned when the > cue had been already pushed, but this method would return false in that case . > > There should also be text about the expectations around start_time and > successive AddCue() calls. For example, start_time is expected to be > monotonically increasing across calls unless there is an intervening Reset(). Done. https://codereview.chromium.org/151103005/diff/50001/media/base/text_ranges.h... media/base/text_ranges.h:50: // matches the seek time. On 2014/02/05 19:09:36, acolwell wrote: > nit: Technically this isn't a seek time. It's the start time of the first cue > after a seek. You should reword this in terms of Reset() instead of seek since > seeking is really outside the scope of this helper class. Done. https://codereview.chromium.org/151103005/diff/50001/media/base/text_ranges.h... media/base/text_ranges.h:58: // The time range to which we bind following a seek. On 2014/02/05 19:09:36, acolwell wrote: > nit: s/seek/Reset()/ Done. https://codereview.chromium.org/151103005/diff/50001/media/base/text_ranges_u... File media/base/text_ranges_unittest.cc (right): https://codereview.chromium.org/151103005/diff/50001/media/base/text_ranges_u... media/base/text_ranges_unittest.cc:18: EXPECT_EQ(r.Present(base::TimeDelta::FromSeconds(5)), 1); On 2014/02/05 19:09:36, acolwell wrote: > Please remove the Present() calls and Present() itself. They just clutter the > tests. You shouldn't need them since production code doesn't access them and you > should be able to verify proper behavior solely with combinations of addCue() > and Reset() calls. > > The one exception I can think of is if you want to make sure that range merging > is happening properly. In that case having a simple size_t > RangeCountForTesting() const method that returns the number of ranges should be > sufficient. You should definitely use this sparingly though and not include the > verification in every test. Just in a few key focused ones. Done.
Getting pretty close. Just a few relatively minor comments. https://codereview.chromium.org/151103005/diff/120001/media/base/text_ranges.cc File media/base/text_ranges.cc (right): https://codereview.chromium.org/151103005/diff/120001/media/base/text_ranges.... media/base/text_ranges.cc:65: // We have walked off the curr range, and onto the next one. nit:s/curr range/current range/ or s/curr range/|curr_range|/ https://codereview.chromium.org/151103005/diff/120001/media/base/text_ranges.... media/base/text_ranges.cc:67: // ends, and so we coalesce the curr and next ranges. ditto. Either use "current" or the actual variable names with appropriate || wrapping. https://codereview.chromium.org/151103005/diff/120001/media/base/text_ranges.... media/base/text_ranges.cc:74: // Either curr is the last range in the map, or there is a next ditto for this comment as well. https://codereview.chromium.org/151103005/diff/120001/media/base/text_ranges.... media/base/text_ranges.cc:102: RangeMap::iterator next_range_itr) { nit: make const& https://codereview.chromium.org/151103005/diff/120001/media/base/text_ranges.... media/base/text_ranges.cc:112: void TextRanges::Range::SetLastTime(base::TimeDelta start_time) { nit: s/start_time/last_time/ https://codereview.chromium.org/151103005/diff/120001/media/base/text_ranges.h File media/base/text_ranges.h (right): https://codereview.chromium.org/151103005/diff/120001/media/base/text_ranges.... media/base/text_ranges.h:41: struct Range { Convert this to class. Make all the methods public and all the members private. Add the trailing '_' to all the member names and create a "base::TimeDelta last_time() const" accessor since this appears to be the only member that needs to be externally accessed now. https://codereview.chromium.org/151103005/diff/120001/media/base/text_ranges_... File media/base/text_ranges_unittest.cc (right): https://codereview.chromium.org/151103005/diff/120001/media/base/text_ranges_... media/base/text_ranges_unittest.cc:16: EXPECT_TRUE(r.AddCue(base::TimeDelta::FromSeconds(5))); nit: Please add a TextRangesTest class declaration that includes a ranges_ member, "bool AddCue(int start_time)" and "void Reset()" helper methods so you can get rid of all the base::TimeDelta::FromSeconds and r. clutter. You might also want to implement a string based helper similar to what is in SourceBufferStream test to make it easy to concisely test a sequence of AddCue() calls that are all expected to return true. EXPECT_TRUE(AddCue("0 1 2 3")) is a lot easier to read than 4 equivalent AddCue() lines. https://codereview.chromium.org/151103005/diff/120001/media/base/text_ranges_... media/base/text_ranges_unittest.cc:66: EXPECT_FALSE(r.AddCue(base::TimeDelta::FromMilliseconds(1500))); nit: I think you should separate testing the "adding cues to a previous range w/ different timestamps" from the "adding the same cues again" cases so each situation is isolated. The former is more like what ChunkDemuxer will do when overlaps are present and the latter is more like what FFmpegDemuxer will do all the time.
Incorporated Aaron's comments of 2014/02/06. https://codereview.chromium.org/151103005/diff/120001/media/base/text_ranges.cc File media/base/text_ranges.cc (right): https://codereview.chromium.org/151103005/diff/120001/media/base/text_ranges.... media/base/text_ranges.cc:65: // We have walked off the curr range, and onto the next one. On 2014/02/07 01:44:18, acolwell wrote: > nit:s/curr range/current range/ or s/curr range/|curr_range|/ Done. https://codereview.chromium.org/151103005/diff/120001/media/base/text_ranges.... media/base/text_ranges.cc:67: // ends, and so we coalesce the curr and next ranges. On 2014/02/07 01:44:18, acolwell wrote: > ditto. Either use "current" or the actual variable names with appropriate || > wrapping. Done. https://codereview.chromium.org/151103005/diff/120001/media/base/text_ranges.... media/base/text_ranges.cc:74: // Either curr is the last range in the map, or there is a next On 2014/02/07 01:44:18, acolwell wrote: > ditto for this comment as well. Done. https://codereview.chromium.org/151103005/diff/120001/media/base/text_ranges.... media/base/text_ranges.cc:102: RangeMap::iterator next_range_itr) { On 2014/02/07 01:44:18, acolwell wrote: > nit: make const& Done. https://codereview.chromium.org/151103005/diff/120001/media/base/text_ranges.... media/base/text_ranges.cc:112: void TextRanges::Range::SetLastTime(base::TimeDelta start_time) { On 2014/02/07 01:44:18, acolwell wrote: > nit: s/start_time/last_time/ Done. https://codereview.chromium.org/151103005/diff/120001/media/base/text_ranges.h File media/base/text_ranges.h (right): https://codereview.chromium.org/151103005/diff/120001/media/base/text_ranges.... media/base/text_ranges.h:41: struct Range { On 2014/02/07 01:44:18, acolwell wrote: > Convert this to class. Make all the methods public and all the members private. > Add the trailing '_' to all the member names and create a "base::TimeDelta > last_time() const" accessor since this appears to be the only member that needs > to be externally accessed now. Done. https://codereview.chromium.org/151103005/diff/120001/media/base/text_ranges_... File media/base/text_ranges_unittest.cc (right): https://codereview.chromium.org/151103005/diff/120001/media/base/text_ranges_... media/base/text_ranges_unittest.cc:16: EXPECT_TRUE(r.AddCue(base::TimeDelta::FromSeconds(5))); On 2014/02/07 01:44:18, acolwell wrote: > nit: Please add a TextRangesTest class declaration that includes a ranges_ > member, "bool AddCue(int start_time)" and "void Reset()" helper methods so you > can get rid of all the base::TimeDelta::FromSeconds and r. clutter. You might > also want to implement a string based helper similar to what is in > SourceBufferStream test to make it easy to concisely test a sequence of AddCue() > calls that are all expected to return true. > EXPECT_TRUE(AddCue("0 1 2 3")) is a lot easier to read than 4 equivalent > AddCue() lines. Done. https://codereview.chromium.org/151103005/diff/120001/media/base/text_ranges_... media/base/text_ranges_unittest.cc:66: EXPECT_FALSE(r.AddCue(base::TimeDelta::FromMilliseconds(1500))); On 2014/02/07 01:44:18, acolwell wrote: > nit: I think you should separate testing the "adding cues to a previous range w/ > different timestamps" from the "adding the same cues again" cases so each > situation is isolated. The former is more like what ChunkDemuxer will do when > overlaps are present and the latter is more like what FFmpegDemuxer will do all > the time. Done.
lgtm % nits. Thanks for working on this. https://codereview.chromium.org/151103005/diff/160001/media/base/text_ranges.cc File media/base/text_ranges.cc (right): https://codereview.chromium.org/151103005/diff/160001/media/base/text_ranges.... media/base/text_ranges.cc:65: // We have walked off the currrent range, and onto the next one. nit: s/currrent/current/ https://codereview.chromium.org/151103005/diff/160001/media/base/text_ranges.... media/base/text_ranges.cc:74: // Either curr_range is the last range in the map, or there is a nit: s/curr_range/|curr_range|/ here and elsewhere in this comment. https://codereview.chromium.org/151103005/diff/160001/media/base/text_ranges.h File media/base/text_ranges.h (right): https://codereview.chromium.org/151103005/diff/160001/media/base/text_ranges.... media/base/text_ranges.h:16: class MEDIA_EXPORT TextRanges { nit: Add class comment. Probably something around the lines of "Helper class used by the TextRenderer to filter out text cues that have already been passed downstream." https://codereview.chromium.org/151103005/diff/160001/media/base/text_ranges_... File media/base/text_ranges_unittest.cc (right): https://codereview.chromium.org/151103005/diff/160001/media/base/text_ranges_... media/base/text_ranges_unittest.cc:102: EXPECT_FALSE(AddCue(1)); nit: s/1/0/ so using different timestamps doesn't cloud what this is testing. https://codereview.chromium.org/151103005/diff/160001/media/base/text_ranges_... media/base/text_ranges_unittest.cc:140: EXPECT_FALSE(AddCue(1)); nit: Either put an AddCue(1) above or remove this one so that using a different timestamp doesn't cloud what the specific scenario this test is testing.
Incorporated Aaron's comments of 2014/02/07. https://codereview.chromium.org/151103005/diff/160001/media/base/text_ranges.cc File media/base/text_ranges.cc (right): https://codereview.chromium.org/151103005/diff/160001/media/base/text_ranges.... media/base/text_ranges.cc:65: // We have walked off the currrent range, and onto the next one. On 2014/02/07 18:30:52, acolwell wrote: > nit: s/currrent/current/ Done. https://codereview.chromium.org/151103005/diff/160001/media/base/text_ranges.... media/base/text_ranges.cc:74: // Either curr_range is the last range in the map, or there is a On 2014/02/07 18:30:52, acolwell wrote: > nit: s/curr_range/|curr_range|/ here and elsewhere in this comment. Done. https://codereview.chromium.org/151103005/diff/160001/media/base/text_ranges.h File media/base/text_ranges.h (right): https://codereview.chromium.org/151103005/diff/160001/media/base/text_ranges.... media/base/text_ranges.h:16: class MEDIA_EXPORT TextRanges { On 2014/02/07 18:30:52, acolwell wrote: > nit: Add class comment. Probably something around the lines of "Helper class > used by the TextRenderer to filter out text cues that have already been passed > downstream." Done. https://codereview.chromium.org/151103005/diff/160001/media/base/text_ranges_... File media/base/text_ranges_unittest.cc (right): https://codereview.chromium.org/151103005/diff/160001/media/base/text_ranges_... media/base/text_ranges_unittest.cc:102: EXPECT_FALSE(AddCue(1)); On 2014/02/07 18:30:52, acolwell wrote: > nit: s/1/0/ so using different timestamps doesn't cloud what this is testing. Done. https://codereview.chromium.org/151103005/diff/160001/media/base/text_ranges_... media/base/text_ranges_unittest.cc:140: EXPECT_FALSE(AddCue(1)); On 2014/02/07 18:30:52, acolwell wrote: > nit: Either put an AddCue(1) above or remove this one so that using a different > timestamp doesn't cloud what the specific scenario this test is testing. Done.
The CQ bit was checked by matthewjheaney@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/matthewjheaney@chromium.org/151103005/...
The CQ bit was unchecked by commit-bot@chromium.org
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...
The CQ bit was checked by matthewjheaney@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/matthewjheaney@chromium.org/151103005/...
The CQ bit was unchecked by commit-bot@chromium.org
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...
The CQ bit was checked by matthewjheaney@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/matthewjheaney@chromium.org/151103005/...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/matthewjheaney@chromium.org/151103005/...
Message was sent while issue was closed.
Change committed as 249845 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
