Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10228017:
Add initial implementation of SourceBufferStream (Closed)
|
Created:
8 years, 8 months ago by vrk (LEFT CHROMIUM) Modified:
8 years, 7 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd initial implementation of SourceBufferStream
This CL doesn't integrate SourceBufferStream into Chromium yet, but the
functionality is tested in the unit test.
BUG=125092
TEST=media_unittests
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=135441
Patch Set 1 #Patch Set 2 : . #
Total comments: 2
Patch Set 3 : Response to driveby #Patch Set 4 : . #
Total comments: 68
Patch Set 5 : Response to CR #Patch Set 6 : . #
Total comments: 34
Patch Set 7 : Response to CR #
Total comments: 6
Patch Set 8 : win_rel unhappy with bool = int #Patch Set 9 : . #Patch Set 10 : Rebase ToT #Patch Set 11 : Fix win_rel #Patch Set 12 : rebase ToT #
Messages
Total messages: 13 (0 generated)
bombs away!!! It pretty much follows what I wrote in the doc, except the logic for keeping track of the "current/selected" Range was trickier than I thought. Let me know what I can do to make reviewing easier!
drive by https://chromiumcodereview.appspot.com/10228017/diff/2001/media/base/buffers.h File media/base/buffers.h (right): https://chromiumcodereview.appspot.com/10228017/diff/2001/media/base/buffers.... media/base/buffers.h:77: virtual bool IsKeyframe() const { don't inline virtual methods in .h http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts also couldn't virtual + MockDataBuffer be replaced with a call to SetKeyframe() in the unit test?
https://chromiumcodereview.appspot.com/10228017/diff/2001/media/base/buffers.h File media/base/buffers.h (right): https://chromiumcodereview.appspot.com/10228017/diff/2001/media/base/buffers.... media/base/buffers.h:77: virtual bool IsKeyframe() const { On 2012/04/25 22:59:00, scherkus wrote: > don't inline virtual methods in .h > http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts > > also couldn't virtual + MockDataBuffer be replaced with a call to SetKeyframe() > in the unit test? Yeahhh as I explained offline, I was worried that people would use these methods when they're not actually supported yet, but probably not worth worrying about. Devirtualized and got rid of the Mock. We could/should? have ChunkedDemuxer use a special derived class of Buffer that has these methods on it, so as not to pollute the Buffer base class with methods that only ChunkDemuxer + friends care about. But since we're not sure yet if that's the right direction to go in, I'll just keep the methods in Buffer with a TODO.
Here's some initial comments. I'll review the rest of the code tomorrow. https://chromiumcodereview.appspot.com/10228017/diff/7001/media/base/buffers.h File media/base/buffers.h (right): https://chromiumcodereview.appspot.com/10228017/diff/7001/media/base/buffers.... media/base/buffers.h:79: void SetKeyframe(bool is_keyframe) { Could we get away with just providing this info via the constructor and do away with the setter? https://chromiumcodereview.appspot.com/10228017/diff/7001/media/filters/sourc... File media/filters/source_buffer_stream.h (right): https://chromiumcodereview.appspot.com/10228017/diff/7001/media/filters/sourc... media/filters/source_buffer_stream.h:43: // buffers starting from |timestamp|. I believe this should this be "starting from the closest keyframe before |timestamp|". https://chromiumcodereview.appspot.com/10228017/diff/7001/media/filters/sourc... media/filters/source_buffer_stream.h:51: // fufill the request. s/fufill/fulfill https://chromiumcodereview.appspot.com/10228017/diff/7001/media/filters/sourc... media/filters/source_buffer_stream.h:60: class Range { How about calling this SourceBufferRange and placing this in the .cc file? You'll have to update the forward declaration above too. That way you don't have to expose implementation details of this class to other code. https://chromiumcodereview.appspot.com/10228017/diff/7001/media/filters/sourc... media/filters/source_buffer_stream.h:71: void Merge(Range* range); Any reason this and CanMerge can't be const Range& ? https://chromiumcodereview.appspot.com/10228017/diff/7001/media/filters/sourc... media/filters/source_buffer_stream.h:77: // Returns 0 if |timestamp| is in this range, a negative value if Any reason not to use -1 & 1 instead of the whole negative and positive ranges? https://chromiumcodereview.appspot.com/10228017/diff/7001/media/filters/sourc... media/filters/source_buffer_stream.h:84: bool CanSeekTo(base::TimeDelta timestamp) const; The method name and comment don't seem consistent. Is this supposed to tell me whether the range has enough data to allow me to seek to the specified timestamp or whether the range contains a buffer that has the specified timestamp? https://chromiumcodereview.appspot.com/10228017/diff/7001/media/filters/sourc... media/filters/source_buffer_stream.h:88: bool EndOverlaps(const Range* range) const; const Range& ? https://chromiumcodereview.appspot.com/10228017/diff/7001/media/filters/sourc... media/filters/source_buffer_stream.h:122: bool IsRangeListSorted() const; You should probably just change this to a static function within the .cc since it is only used to DCHECK the sorted list invariant. It doesn't really need to be a member method.
This is a pretty good start. Here are the rest of my comments. http://codereview.chromium.org/10228017/diff/7001/media/filters/source_buffer... File media/filters/source_buffer_stream.cc (right): http://codereview.chromium.org/10228017/diff/7001/media/filters/source_buffer... media/filters/source_buffer_stream.cc:9: namespace { We don't do anonymous namespaces in the media code. The static below is sufficient to narrow the scope to this file. http://codereview.chromium.org/10228017/diff/7001/media/filters/source_buffer... media/filters/source_buffer_stream.cc:12: static bool BufferComparitor(scoped_refptr<media::Buffer> first, Comparator http://codereview.chromium.org/10228017/diff/7001/media/filters/source_buffer... media/filters/source_buffer_stream.cc:35: base::TimeDelta start_timestamp = buffers.front()->GetTimestamp(); DCHECK(!buffers.empty())? http://codereview.chromium.org/10228017/diff/7001/media/filters/source_buffer... media/filters/source_buffer_stream.cc:45: return; Do we really want to silently drop this data? Perhaps add a bool return value so that an error can be propagated to JavaScript? http://codereview.chromium.org/10228017/diff/7001/media/filters/source_buffer... media/filters/source_buffer_stream.cc:51: while (itr != ranges_.end()) { nit: consider using for instead of while here http://codereview.chromium.org/10228017/diff/7001/media/filters/source_buffer... media/filters/source_buffer_stream.cc:64: RangeList::iterator current = itr; I don't think current & next actually make things clearer since when next gets assigned, it is equal to current. How about just using itr everywhere since you only care about one iterator position this whole time. http://codereview.chromium.org/10228017/diff/7001/media/filters/source_buffer... media/filters/source_buffer_stream.cc:85: if (range->is_selected_range()) Do we really need is_selected_range()? Can't we just do a (selected_range_ == *next) here? Putting this in Range feels like an encapsulation leak. http://codereview.chromium.org/10228017/diff/7001/media/filters/source_buffer... media/filters/source_buffer_stream.cc:110: while (itr != ranges_.end()) { nit: use for here http://codereview.chromium.org/10228017/diff/7001/media/filters/source_buffer... media/filters/source_buffer_stream.cc:128: if (!selected_range_) nit: Can combine with the return below. selected_range_ && selected_range_->GetNextBuffer(out_buffer); http://codereview.chromium.org/10228017/diff/7001/media/filters/source_buffer... media/filters/source_buffer_stream.cc:200: DCHECK(result != keyframe_map_.begin()); Shouldn't this be before the if? Putting it here assumes that keyframe_map_.begin()->first can never match timestamp. It isn't clear to me that's a valid assumption. http://codereview.chromium.org/10228017/diff/7001/media/filters/source_buffer... media/filters/source_buffer_stream.cc:209: if (next_buffer_index_ >= static_cast<int>(buffers_.size())) I think you need a next_buffer_index_ < 0 check here. Should this and the existing check be DCHECKs? http://codereview.chromium.org/10228017/diff/7001/media/filters/source_buffer... media/filters/source_buffer_stream.cc:228: if (!buffers_.empty()) Is it possible for buffers_.empty() to be true? It seems weird to merge into an empty range. http://codereview.chromium.org/10228017/diff/7001/media/filters/source_buffer... media/filters/source_buffer_stream.cc:244: return -1; This and the line below seems reversed from what the comment in .h says. http://codereview.chromium.org/10228017/diff/7001/media/filters/source_buffer... media/filters/source_buffer_stream.cc:255: return buffers_.front()->GetTimestamp() <= timestamp && Use BufferedStart() & BufferedEnd() here & drop the condition above? http://codereview.chromium.org/10228017/diff/7001/media/filters/source_buffer... media/filters/source_buffer_stream.cc:278: if (buffers_.empty()) Should this be a DCHECK? http://codereview.chromium.org/10228017/diff/7001/media/filters/source_buffer... media/filters/source_buffer_stream.cc:281: return BufferedEnd() + buffers_.back()->GetDuration() / 3; You might want to add a comment here. I realize this is a made up value, but it wasn't clear to me initially that this is actually 1/3 of a duration past the end of the last buffer in the queue. You should also probably put a DCHECK in the append code somewhere that makes sure the the GetDuration() always returns something > 0 & != kNoTimestamp(). http://codereview.chromium.org/10228017/diff/7001/media/filters/source_buffer... media/filters/source_buffer_stream.cc:287: is_selected_range_ = false; reset next_buffer_index_ too? http://codereview.chromium.org/10228017/diff/7001/media/filters/source_buffer... File media/filters/source_buffer_stream_unittest.cc (right): http://codereview.chromium.org/10228017/diff/7001/media/filters/source_buffer... media/filters/source_buffer_stream_unittest.cc:14: static const int kDefaultKeyframesPerSecond = 15; nit: I'd make this 1 to make it more realistic http://codereview.chromium.org/10228017/diff/7001/media/filters/source_buffer... media/filters/source_buffer_stream_unittest.cc:63: while (actual_itr != actual_times.end()) { nit: use for http://codereview.chromium.org/10228017/diff/7001/media/filters/source_buffer... media/filters/source_buffer_stream_unittest.cc:65: if (expected_itr == expected_times.end()) Should you just put expected_itr != expected_times.end() in the loop condition instead of putting this code here? http://codereview.chromium.org/10228017/diff/7001/media/filters/source_buffer... media/filters/source_buffer_stream_unittest.cc:83: while (buffers_received < number_of_buffers) { nit:use for http://codereview.chromium.org/10228017/diff/7001/media/filters/source_buffer... media/filters/source_buffer_stream_unittest.cc:88: // Skip earlier buffers. Do we really want to skip these buffers? Don't we want to verify the seeking to the nearest keyframe logic too? http://codereview.chromium.org/10228017/diff/7001/media/filters/source_buffer... media/filters/source_buffer_stream_unittest.cc:181: SetStreamInfo(30, 30); Why are you basically setting "all buffers are keyframes" here? http://codereview.chromium.org/10228017/diff/7001/media/filters/source_buffer... media/filters/source_buffer_stream_unittest.cc:328: AppendBuffers(2, 6); I'm not sure about this case. If the frame at position 3 isn't a keyframe then I don't think this is the right behavior. This is the exact case the track buffer was meant to handle. It isn't clear to me that this test properly captures the subtlety of what happens when you overwrite the current position. http://codereview.chromium.org/10228017/diff/7001/media/filters/source_buffer... media/filters/source_buffer_stream_unittest.cc:378: I think you should add a test case that seeks to a position that doesn't exactly match a timestamp on a buffer and make sure that it starts at the proper keyframe.
Thanks for all the helpful comments! https://chromiumcodereview.appspot.com/10228017/diff/7001/media/base/buffers.h File media/base/buffers.h (right): https://chromiumcodereview.appspot.com/10228017/diff/7001/media/base/buffers.... media/base/buffers.h:79: void SetKeyframe(bool is_keyframe) { On 2012/04/26 00:52:34, acolwell wrote: > Could we get away with just providing this info via the constructor and do away > with the setter? Done, but it's a little icky since I have to add a DataBuffer constructor as well. Let me know if you still prefer it this way. https://chromiumcodereview.appspot.com/10228017/diff/7001/media/filters/sourc... File media/filters/source_buffer_stream.cc (right): https://chromiumcodereview.appspot.com/10228017/diff/7001/media/filters/sourc... media/filters/source_buffer_stream.cc:9: namespace { On 2012/04/26 19:51:38, acolwell wrote: > We don't do anonymous namespaces in the media code. The static below is > sufficient to narrow the scope to this file. Done. https://chromiumcodereview.appspot.com/10228017/diff/7001/media/filters/sourc... media/filters/source_buffer_stream.cc:12: static bool BufferComparitor(scoped_refptr<media::Buffer> first, On 2012/04/26 19:51:38, acolwell wrote: > Comparator Done. https://chromiumcodereview.appspot.com/10228017/diff/7001/media/filters/sourc... media/filters/source_buffer_stream.cc:35: base::TimeDelta start_timestamp = buffers.front()->GetTimestamp(); On 2012/04/26 19:51:38, acolwell wrote: > DCHECK(!buffers.empty())? Done. https://chromiumcodereview.appspot.com/10228017/diff/7001/media/filters/sourc... media/filters/source_buffer_stream.cc:45: return; On 2012/04/26 19:51:38, acolwell wrote: > Do we really want to silently drop this data? Perhaps add a bool return value so > that an error can be propagated to JavaScript? Good idea. Bool added! https://chromiumcodereview.appspot.com/10228017/diff/7001/media/filters/sourc... media/filters/source_buffer_stream.cc:51: while (itr != ranges_.end()) { On 2012/04/26 19:51:38, acolwell wrote: > nit: consider using for instead of while here Done. https://chromiumcodereview.appspot.com/10228017/diff/7001/media/filters/sourc... media/filters/source_buffer_stream.cc:64: RangeList::iterator current = itr; On 2012/04/26 19:51:38, acolwell wrote: > I don't think current & next actually make things clearer since when next gets > assigned, it is equal to current. How about just using itr everywhere since you > only care about one iterator position this whole time. Done. https://chromiumcodereview.appspot.com/10228017/diff/7001/media/filters/sourc... media/filters/source_buffer_stream.cc:85: if (range->is_selected_range()) On 2012/04/26 19:51:38, acolwell wrote: > Do we really need is_selected_range()? Can't we just do a (selected_range_ == > *next) here? Putting this in Range feels like an encapsulation leak. Yeah I didn't like that either! Removed, thanks! https://chromiumcodereview.appspot.com/10228017/diff/7001/media/filters/sourc... media/filters/source_buffer_stream.cc:110: while (itr != ranges_.end()) { On 2012/04/26 19:51:38, acolwell wrote: > nit: use for here Done, and cleaned up some surrounding logic. https://chromiumcodereview.appspot.com/10228017/diff/7001/media/filters/sourc... media/filters/source_buffer_stream.cc:128: if (!selected_range_) On 2012/04/26 19:51:38, acolwell wrote: > nit: Can combine with the return below. > selected_range_ && selected_range_->GetNextBuffer(out_buffer); Done. https://chromiumcodereview.appspot.com/10228017/diff/7001/media/filters/sourc... media/filters/source_buffer_stream.cc:200: DCHECK(result != keyframe_map_.begin()); On 2012/04/26 19:51:38, acolwell wrote: > Shouldn't this be before the if? Putting it here assumes that > keyframe_map_.begin()->first can never match timestamp. It isn't clear to me > that's a valid assumption. Talked offline! BTW, I confirmed that if there's only one element, and |timestamp| is after that element, then result is keyframe_map_.end(), and result-- gets you to the right place. For example: keyframe_map_ = { (t=4, 0) } result = keyframe_map_.lower_bound(t=5) // result is keyframe_map_.end() result--; // result is (t=4, 0) Of course, in this case: keyframe_map_ = { (t=4, 0) } result = keyframe_map_.lower_bound(t=3) // result is (t=4, 0) But this should never happen since the client should have appended the keyframe for t=3 before appending t=3, which is why there's a DCHECK. https://chromiumcodereview.appspot.com/10228017/diff/7001/media/filters/sourc... media/filters/source_buffer_stream.cc:209: if (next_buffer_index_ >= static_cast<int>(buffers_.size())) On 2012/04/26 19:51:38, acolwell wrote: > I think you need a next_buffer_index_ < 0 check here. Yes, thanks! Done. > Should this and the existing check be DCHECKs? Chatted offline; the < 0 should be a DCHECK, he "next_buffer_index_ >= static_cast<int>(buffers_.size()" should *not* be a DCHECK, since this is our indication that we're stalling. https://chromiumcodereview.appspot.com/10228017/diff/7001/media/filters/sourc... media/filters/source_buffer_stream.cc:228: if (!buffers_.empty()) On 2012/04/26 19:51:38, acolwell wrote: > Is it possible for buffers_.empty() to be true? It seems weird to merge into an > empty range. You're right, shouldn't be true. I'll DCHECK instead. https://chromiumcodereview.appspot.com/10228017/diff/7001/media/filters/sourc... media/filters/source_buffer_stream.cc:244: return -1; On 2012/04/26 19:51:38, acolwell wrote: > This and the line below seems reversed from what the comment in .h says. Oops, you're right! Originally I had done this backwards, then fixed the code but not the comment :) fixed! https://chromiumcodereview.appspot.com/10228017/diff/7001/media/filters/sourc... media/filters/source_buffer_stream.cc:255: return buffers_.front()->GetTimestamp() <= timestamp && On 2012/04/26 19:51:38, acolwell wrote: > Use BufferedStart() & BufferedEnd() here & drop the condition above? Done. https://chromiumcodereview.appspot.com/10228017/diff/7001/media/filters/sourc... media/filters/source_buffer_stream.cc:278: if (buffers_.empty()) On 2012/04/26 19:51:38, acolwell wrote: > Should this be a DCHECK? Done. https://chromiumcodereview.appspot.com/10228017/diff/7001/media/filters/sourc... media/filters/source_buffer_stream.cc:281: return BufferedEnd() + buffers_.back()->GetDuration() / 3; On 2012/04/26 19:51:38, acolwell wrote: > You might want to add a comment here. I realize this is a made up value, but it > wasn't clear to me initially that this is actually 1/3 of a duration past the > end of the last buffer in the queue. Tried to write a comment, let me know if it doesn't make sense! > You should also probably put a DCHECK in the append code somewhere that makes > sure the the GetDuration() always returns something > 0 & != kNoTimestamp(). Done. https://chromiumcodereview.appspot.com/10228017/diff/7001/media/filters/sourc... media/filters/source_buffer_stream.cc:287: is_selected_range_ = false; On 2012/04/26 19:51:38, acolwell wrote: > reset next_buffer_index_ too? Done. https://chromiumcodereview.appspot.com/10228017/diff/7001/media/filters/sourc... File media/filters/source_buffer_stream.h (right): https://chromiumcodereview.appspot.com/10228017/diff/7001/media/filters/sourc... media/filters/source_buffer_stream.h:43: // buffers starting from |timestamp|. On 2012/04/26 00:52:34, acolwell wrote: > I believe this should this be "starting from the closest keyframe before > |timestamp|". Done. https://chromiumcodereview.appspot.com/10228017/diff/7001/media/filters/sourc... media/filters/source_buffer_stream.h:51: // fufill the request. On 2012/04/26 00:52:34, acolwell wrote: > s/fufill/fulfill Done. https://chromiumcodereview.appspot.com/10228017/diff/7001/media/filters/sourc... media/filters/source_buffer_stream.h:60: class Range { On 2012/04/26 00:52:34, acolwell wrote: > How about calling this SourceBufferRange and placing this in the .cc file? > You'll have to update the forward declaration above too. That way you don't have > to expose implementation details of this class to other code. Done. https://chromiumcodereview.appspot.com/10228017/diff/7001/media/filters/sourc... media/filters/source_buffer_stream.h:71: void Merge(Range* range); On 2012/04/26 00:52:34, acolwell wrote: > Any reason this and CanMerge can't be const Range& ? Changed things to const Range& where it made sense! In Merge(), I'm modifying |range| by removing all its elements. I don't actually have to do that since SouceBufferStream deletes Range immediately after Merge(), but I thought it made more sense logically. Let me know if you want me to change the behavior. https://chromiumcodereview.appspot.com/10228017/diff/7001/media/filters/sourc... media/filters/source_buffer_stream.h:77: // Returns 0 if |timestamp| is in this range, a negative value if On 2012/04/26 00:52:34, acolwell wrote: > Any reason not to use -1 & 1 instead of the whole negative and positive ranges? Changed to -1 and 1. https://chromiumcodereview.appspot.com/10228017/diff/7001/media/filters/sourc... media/filters/source_buffer_stream.h:84: bool CanSeekTo(base::TimeDelta timestamp) const; On 2012/04/26 00:52:34, acolwell wrote: > The method name and comment don't seem consistent. Is this supposed to tell me > whether the range has enough data to allow me to seek to the specified timestamp > or whether the range contains a buffer that has the specified timestamp? The former. Fixed comment. https://chromiumcodereview.appspot.com/10228017/diff/7001/media/filters/sourc... media/filters/source_buffer_stream.h:88: bool EndOverlaps(const Range* range) const; On 2012/04/26 00:52:34, acolwell wrote: > const Range& ? Done. https://chromiumcodereview.appspot.com/10228017/diff/7001/media/filters/sourc... media/filters/source_buffer_stream.h:122: bool IsRangeListSorted() const; On 2012/04/26 00:52:34, acolwell wrote: > You should probably just change this to a static function within the .cc since > it is only used to DCHECK the sorted list invariant. It doesn't really need to > be a member method. Done. https://chromiumcodereview.appspot.com/10228017/diff/7001/media/filters/sourc... File media/filters/source_buffer_stream_unittest.cc (right): https://chromiumcodereview.appspot.com/10228017/diff/7001/media/filters/sourc... media/filters/source_buffer_stream_unittest.cc:14: static const int kDefaultKeyframesPerSecond = 15; On 2012/04/26 19:51:38, acolwell wrote: > nit: I'd make this 1 to make it more realistic Set to 6; modified some other tests as necessary. https://chromiumcodereview.appspot.com/10228017/diff/7001/media/filters/sourc... media/filters/source_buffer_stream_unittest.cc:63: while (actual_itr != actual_times.end()) { On 2012/04/26 19:51:38, acolwell wrote: > nit: use for Done. https://chromiumcodereview.appspot.com/10228017/diff/7001/media/filters/sourc... media/filters/source_buffer_stream_unittest.cc:65: if (expected_itr == expected_times.end()) On 2012/04/26 19:51:38, acolwell wrote: > Should you just put expected_itr != expected_times.end() in the loop condition > instead of putting this code here? Yeah you're right, I already essentially do the check for this on line 57. Done. https://chromiumcodereview.appspot.com/10228017/diff/7001/media/filters/sourc... media/filters/source_buffer_stream_unittest.cc:83: while (buffers_received < number_of_buffers) { On 2012/04/26 19:51:38, acolwell wrote: > nit:use for Done. https://chromiumcodereview.appspot.com/10228017/diff/7001/media/filters/sourc... media/filters/source_buffer_stream_unittest.cc:88: // Skip earlier buffers. On 2012/04/26 19:51:38, acolwell wrote: > Do we really want to skip these buffers? Don't we want to verify the seeking to > the nearest keyframe logic too? Changed and cleaned up some logic as well. Also this helped me find an off by one bug! (In Merge(), I should not subtract 1.) https://chromiumcodereview.appspot.com/10228017/diff/7001/media/filters/sourc... media/filters/source_buffer_stream_unittest.cc:181: SetStreamInfo(30, 30); On 2012/04/26 19:51:38, acolwell wrote: > Why are you basically setting "all buffers are keyframes" here? Talked offline. Added comment + extra test. https://chromiumcodereview.appspot.com/10228017/diff/7001/media/filters/sourc... media/filters/source_buffer_stream_unittest.cc:328: AppendBuffers(2, 6); On 2012/04/26 19:51:38, acolwell wrote: > I'm not sure about this case. If the frame at position 3 isn't a keyframe then I > don't think this is the right behavior. This is the exact case the track buffer > was meant to handle. It isn't clear to me that this test properly captures the > subtlety of what happens when you overwrite the current position. Talked offline; modified test a bit and wrote a comment. https://chromiumcodereview.appspot.com/10228017/diff/7001/media/filters/sourc... media/filters/source_buffer_stream_unittest.cc:378: On 2012/04/26 19:51:38, acolwell wrote: > I think you should add a test case that seeks to a position that doesn't exactly > match a timestamp on a buffer and make sure that it starts at the proper > keyframe. Added a test, thought it looks a little funny. Let me know if you want me to tweak it!
Looks good. mostly minor comments at this point. http://codereview.chromium.org/10228017/diff/9011/media/base/buffers.h File media/base/buffers.h (right): http://codereview.chromium.org/10228017/diff/9011/media/base/buffers.h#newcode77 media/base/buffers.h:77: // Put keyframe stuff inside a derived class of Buffer? I think you should remove this TODO. ISTM if we really were worried about this we'd do this right now instead of temporarily putting this method here. http://codereview.chromium.org/10228017/diff/9011/media/base/buffers.h#newcode83 media/base/buffers.h:83: Buffer(base::TimeDelta timestamp, base::TimeDelta duration, bool is_keyframe); I think we should be able to get away with just adding the parameter to the existing constructor. It doesn't look like there are too many call sites to update. You should probably make the buffer changes a different CL so we can focus on just those changes. http://codereview.chromium.org/10228017/diff/9011/media/base/data_buffer.h File media/base/data_buffer.h (right): http://codereview.chromium.org/10228017/diff/9011/media/base/data_buffer.h#ne... media/base/data_buffer.h:27: // Put keyframe stuff inside a derived class of Buffer? Remove comment since we are already making this decision right now. http://codereview.chromium.org/10228017/diff/9011/media/filters/source_buffer... File media/filters/source_buffer_stream.cc (right): http://codereview.chromium.org/10228017/diff/9011/media/filters/source_buffer... media/filters/source_buffer_stream.cc:28: void Append(const BufferQueue& buffers); nit: add documentation for these methods. http://codereview.chromium.org/10228017/diff/9011/media/filters/source_buffer... media/filters/source_buffer_stream.cc:46: int BelongsToRange(base::TimeDelta timestamp) const; nit: This is really minor, but mind if we switch the values? 1 for before and -1 for after seems backwards to me. Either that or change the comment text to "1 if this range is after |timestamp|". I slightly prefer the former just because it indicates how the parameter relates to the range instead of the other way around. http://codereview.chromium.org/10228017/diff/9011/media/filters/source_buffer... media/filters/source_buffer_stream.cc:57: void AppendToEnd(const BufferQueue& buffers); docs for these methods. http://codereview.chromium.org/10228017/diff/9011/media/filters/source_buffer... media/filters/source_buffer_stream.cc:98: selected_range_(NULL) { seek_buffer_timestamp_(kNoTimestamp()) ? http://codereview.chromium.org/10228017/diff/9011/media/filters/source_buffer... media/filters/source_buffer_stream.cc:127: for (itr = ranges_.begin(); itr != ranges_.end();itr++) { nit: space after ; http://codereview.chromium.org/10228017/diff/9011/media/filters/source_buffer... media/filters/source_buffer_stream.cc:129: if (range_value > 0) Please add a comment why we are breaking here. I'm assuming it's because we've discovered that the timestamp is actually before the current range. http://codereview.chromium.org/10228017/diff/9011/media/filters/source_buffer... media/filters/source_buffer_stream.cc:139: if (create_new_range) I think you might be able to reduce the subtlety of what is going on here by replacing the create_new_range declaration with "SourceBufferRange* range = NULL;" and then basing the create & insert on whether range is still NULL after the loop exits. http://codereview.chromium.org/10228017/diff/9011/media/filters/source_buffer... media/filters/source_buffer_stream.cc:140: itr = ranges_.insert(itr, new SourceBufferRange()); Before creating the new range object you should add a check here to make sure that the first buffer is a keyframe . If it isn't this function should fail return false. We definitely want a test case for this code path. http://codereview.chromium.org/10228017/diff/9011/media/filters/source_buffer... media/filters/source_buffer_stream.cc:277: return std::make_pair<base::TimeDelta, base::TimeDelta>( nit: Lint says you don't need the template parameters here. Is that true? http://codereview.chromium.org/10228017/diff/9011/media/filters/source_buffer... media/filters/source_buffer_stream.cc:281: void SourceBufferRange::Merge(SourceBufferRange* range, How about renaming this to AppendRange() instead of Merge()? That way making range const SourceBufferRange& seems less awkward and we don't have to do the unnecessary range->Reset() at the bottom. I think that's also the only call to Reset() so that method can go away too. http://codereview.chromium.org/10228017/diff/9011/media/filters/source_buffer... media/filters/source_buffer_stream.cc:310: return BufferedStart() <= timestamp && BufferedEnd() > timestamp; !keyframe_map_.empty() && http://codereview.chromium.org/10228017/diff/9011/media/filters/source_buffer... File media/filters/source_buffer_stream.h (right): http://codereview.chromium.org/10228017/diff/9011/media/filters/source_buffer... media/filters/source_buffer_stream.h:9: #include <list> nit: lint says you should add the include for std::pair here. I think it's #include <utility> http://codereview.chromium.org/10228017/diff/9011/media/filters/source_buffer... File media/filters/source_buffer_stream_unittest.cc (right): http://codereview.chromium.org/10228017/diff/9011/media/filters/source_buffer... media/filters/source_buffer_stream_unittest.cc:40: stream_.Append(queue); EXPECT_TRUE() or return stream_.Append(queue) & EXPECT_XXX at caller? http://codereview.chromium.org/10228017/diff/9011/media/filters/source_buffer... media/filters/source_buffer_stream_unittest.cc:49: return std::make_pair<base::TimeDelta, base::TimeDelta>( nit: lint seems to believe that the template parameters can be removed. Is this true?
Responded and rebased to CL 10269022. https://chromiumcodereview.appspot.com/10228017/diff/9011/media/base/buffers.h File media/base/buffers.h (right): https://chromiumcodereview.appspot.com/10228017/diff/9011/media/base/buffers.... media/base/buffers.h:77: // Put keyframe stuff inside a derived class of Buffer? On 2012/04/29 18:13:06, acolwell wrote: > I think you should remove this TODO. ISTM if we really were worried about this > we'd do this right now instead of temporarily putting this method here. Done. https://chromiumcodereview.appspot.com/10228017/diff/9011/media/base/buffers.... media/base/buffers.h:83: Buffer(base::TimeDelta timestamp, base::TimeDelta duration, bool is_keyframe); On 2012/04/29 18:13:06, acolwell wrote: > I think we should be able to get away with just adding the parameter to the > existing constructor. It doesn't look like there are too many call sites to > update. > You should probably make the buffer changes a different CL so we can focus on > just those changes. Done, CL here: https://chromiumcodereview.appspot.com/10269022/ https://chromiumcodereview.appspot.com/10228017/diff/9011/media/base/data_buf... File media/base/data_buffer.h (right): https://chromiumcodereview.appspot.com/10228017/diff/9011/media/base/data_buf... media/base/data_buffer.h:27: // Put keyframe stuff inside a derived class of Buffer? On 2012/04/29 18:13:06, acolwell wrote: > Remove comment since we are already making this decision right now. Done. https://chromiumcodereview.appspot.com/10228017/diff/9011/media/filters/sourc... File media/filters/source_buffer_stream.cc (right): https://chromiumcodereview.appspot.com/10228017/diff/9011/media/filters/sourc... media/filters/source_buffer_stream.cc:28: void Append(const BufferQueue& buffers); On 2012/04/29 18:13:06, acolwell wrote: > nit: add documentation for these methods. Done. https://chromiumcodereview.appspot.com/10228017/diff/9011/media/filters/sourc... media/filters/source_buffer_stream.cc:46: int BelongsToRange(base::TimeDelta timestamp) const; On 2012/04/29 18:13:06, acolwell wrote: > nit: This is really minor, but mind if we switch the values? 1 for before and -1 > for after seems backwards to me. Either that or change the comment text to "1 if > this range is after |timestamp|". I slightly prefer the former just because it > indicates how the parameter relates to the range instead of the other way > around. Yeah, now that I've explained it outloud, it makes more sense the other way around :) changed! https://chromiumcodereview.appspot.com/10228017/diff/9011/media/filters/sourc... media/filters/source_buffer_stream.cc:57: void AppendToEnd(const BufferQueue& buffers); On 2012/04/29 18:13:06, acolwell wrote: > docs for these methods. Done. https://chromiumcodereview.appspot.com/10228017/diff/9011/media/filters/sourc... media/filters/source_buffer_stream.cc:98: selected_range_(NULL) { On 2012/04/29 18:13:06, acolwell wrote: > seek_buffer_timestamp_(kNoTimestamp()) ? Yes, thanks for the catch! Done. https://chromiumcodereview.appspot.com/10228017/diff/9011/media/filters/sourc... media/filters/source_buffer_stream.cc:127: for (itr = ranges_.begin(); itr != ranges_.end();itr++) { On 2012/04/29 18:13:06, acolwell wrote: > nit: space after ; Done. https://chromiumcodereview.appspot.com/10228017/diff/9011/media/filters/sourc... media/filters/source_buffer_stream.cc:129: if (range_value > 0) On 2012/04/29 18:13:06, acolwell wrote: > Please add a comment why we are breaking here. I'm assuming it's because we've > discovered that the timestamp is actually before the current range. Done. https://chromiumcodereview.appspot.com/10228017/diff/9011/media/filters/sourc... media/filters/source_buffer_stream.cc:139: if (create_new_range) On 2012/04/29 18:13:06, acolwell wrote: > I think you might be able to reduce the subtlety of what is going on here by > replacing the create_new_range declaration with "SourceBufferRange* range = > NULL;" and then basing the create & insert on whether range is still NULL after > the loop exits. Good idea! Done. https://chromiumcodereview.appspot.com/10228017/diff/9011/media/filters/sourc... media/filters/source_buffer_stream.cc:140: itr = ranges_.insert(itr, new SourceBufferRange()); On 2012/04/29 18:13:06, acolwell wrote: > Before creating the new range object you should add a check here to make sure > that the first buffer is a keyframe . If it isn't this function should fail > return false. We definitely want a test case for this code path. Done and modified/added test. https://chromiumcodereview.appspot.com/10228017/diff/9011/media/filters/sourc... media/filters/source_buffer_stream.cc:277: return std::make_pair<base::TimeDelta, base::TimeDelta>( On 2012/04/29 18:13:06, acolwell wrote: > nit: Lint says you don't need the template parameters here. Is that true? D'oh, yes, this is true! You can omit the template arguments if they can be deduced. I omitted them above, actually! Removed. https://chromiumcodereview.appspot.com/10228017/diff/9011/media/filters/sourc... media/filters/source_buffer_stream.cc:281: void SourceBufferRange::Merge(SourceBufferRange* range, On 2012/04/29 18:13:06, acolwell wrote: > How about renaming this to AppendRange() instead of Merge()? That way making > range const SourceBufferRange& seems less awkward and we don't have to do the > unnecessary range->Reset() at the bottom. > > I think that's also the only call to Reset() so that method can go away too. Done. https://chromiumcodereview.appspot.com/10228017/diff/9011/media/filters/sourc... media/filters/source_buffer_stream.cc:310: return BufferedStart() <= timestamp && BufferedEnd() > timestamp; On 2012/04/29 18:13:06, acolwell wrote: > !keyframe_map_.empty() && Done. https://chromiumcodereview.appspot.com/10228017/diff/9011/media/filters/sourc... File media/filters/source_buffer_stream.h (right): https://chromiumcodereview.appspot.com/10228017/diff/9011/media/filters/sourc... media/filters/source_buffer_stream.h:9: #include <list> On 2012/04/29 18:13:06, acolwell wrote: > nit: lint says you should add the include for std::pair here. I think it's > #include <utility> Done. https://chromiumcodereview.appspot.com/10228017/diff/9011/media/filters/sourc... File media/filters/source_buffer_stream_unittest.cc (right): https://chromiumcodereview.appspot.com/10228017/diff/9011/media/filters/sourc... media/filters/source_buffer_stream_unittest.cc:40: stream_.Append(queue); On 2012/04/29 18:13:06, acolwell wrote: > EXPECT_TRUE() or return stream_.Append(queue) & EXPECT_XXX at caller? Done. https://chromiumcodereview.appspot.com/10228017/diff/9011/media/filters/sourc... media/filters/source_buffer_stream_unittest.cc:49: return std::make_pair<base::TimeDelta, base::TimeDelta>( On 2012/04/29 18:13:06, acolwell wrote: > nit: lint seems to believe that the template parameters can be removed. Is this > true? Done.
LGTM % nits https://chromiumcodereview.appspot.com/10228017/diff/28003/media/filters/sour... File media/filters/source_buffer_stream.cc (right): https://chromiumcodereview.appspot.com/10228017/diff/28003/media/filters/sour... media/filters/source_buffer_stream.cc:331: nit:remove empty line https://chromiumcodereview.appspot.com/10228017/diff/28003/media/filters/sour... File media/filters/source_buffer_stream.h (right): https://chromiumcodereview.appspot.com/10228017/diff/28003/media/filters/sour... media/filters/source_buffer_stream.h:14: #include "media/base/media_export.h" nit: include order
http://codereview.chromium.org/10228017/diff/28003/media/filters/source_buffe... File media/filters/source_buffer_stream.cc (right): http://codereview.chromium.org/10228017/diff/28003/media/filters/source_buffe... media/filters/source_buffer_stream.cc:188: while (itr != ranges_.end() && range->EndOverlaps(*(*itr))) { nit: You should go back to const Range* here and in the other methods like you had before. I think the *(*itr) will just confuse people for no good reason. I don't know what I was thinking earlier. sorry. :)
Thanks for reviewing! Will land when trybots are happy. https://chromiumcodereview.appspot.com/10228017/diff/28003/media/filters/sour... File media/filters/source_buffer_stream.cc (right): https://chromiumcodereview.appspot.com/10228017/diff/28003/media/filters/sour... media/filters/source_buffer_stream.cc:188: while (itr != ranges_.end() && range->EndOverlaps(*(*itr))) { On 2012/05/02 00:01:43, acolwell wrote: > nit: You should go back to const Range* here and in the other methods like you > had before. I think the *(*itr) will just confuse people for no good reason. I > don't know what I was thinking earlier. sorry. :) I disagree! I stand by your original comment :) const ref is in line with Chromium style, and so unless it needs to be a const* (i.e. if NULL were a valid value to pass into these methods) we should use const ref. https://chromiumcodereview.appspot.com/10228017/diff/28003/media/filters/sour... media/filters/source_buffer_stream.cc:331: On 2012/05/01 23:44:59, acolwell wrote: > nit:remove empty line Done. https://chromiumcodereview.appspot.com/10228017/diff/28003/media/filters/sour... File media/filters/source_buffer_stream.h (right): https://chromiumcodereview.appspot.com/10228017/diff/28003/media/filters/sour... media/filters/source_buffer_stream.h:14: #include "media/base/media_export.h" On 2012/05/01 23:44:59, acolwell wrote: > nit: include order Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vrk@chromium.org/10228017/59003
Try job failure for 10228017-59003 on win_rel for step "update". http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu... Step "update" is always a major failure. Look at the try server FAQ for more details. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
