Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1109)

Issue 10829108: Cap sourceBuffered() on duration and truncate duration on EoS (Closed)

Created:
8 years, 4 months ago by vrk (LEFT CHROMIUM)
Modified:
8 years, 4 months ago
CC:
chromium-reviews, feature-media-reviews_chromium.org
Visibility:
Public.

Description

Cap sourceBuffered() on duration and truncate duration on EoS Adds logic + test to truncate the duration on Media Source streams when EndOfStream is signalled. Also adds some logic to make sure buffered streams never exceed the reported duration. BUG=139899 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=149702

Patch Set 1 #

Patch Set 2 : . #

Patch Set 3 : Updated test #

Total comments: 14

Patch Set 4 : rebase ToT #

Patch Set 5 : rebase ToT #

Unified diffs Side-by-side diffs Delta from patch set Stats (+116 lines, -40 lines) Patch
M media/base/demuxer_stream.h View 1 chunk +0 lines, -3 lines 0 comments Download
M media/base/ranges.h View 2 chunks +2 lines, -2 lines 0 comments Download
M media/filters/chunk_demuxer.h View 1 2 3 4 1 chunk +7 lines, -0 lines 0 comments Download
M media/filters/chunk_demuxer.cc View 1 2 3 4 9 chunks +44 lines, -17 lines 0 comments Download
M media/filters/chunk_demuxer_unittest.cc View 1 2 3 4 8 chunks +44 lines, -7 lines 0 comments Download
M media/filters/dummy_demuxer.h View 1 2 3 1 chunk +0 lines, -1 line 0 comments Download
M media/filters/dummy_demuxer.cc View 1 2 3 1 chunk +0 lines, -4 lines 0 comments Download
M media/filters/ffmpeg_demuxer.h View 1 2 3 2 chunks +8 lines, -1 line 0 comments Download
M media/filters/ffmpeg_demuxer.cc View 1 2 3 3 chunks +11 lines, -5 lines 0 comments Download

Messages

Total messages: 3 (0 generated)
vrk (LEFT CHROMIUM)
This CL still doesn't implement the explicit duration stuff; will implement that in the next ...
8 years, 4 months ago (2012-08-01 00:10:06 UTC) #1
acolwell GONE FROM CHROMIUM
LGTM % nits https://chromiumcodereview.appspot.com/10829108/diff/3010/media/filters/chunk_demuxer.cc File media/filters/chunk_demuxer.cc (right): https://chromiumcodereview.appspot.com/10829108/diff/3010/media/filters/chunk_demuxer.cc#newcode204 media/filters/chunk_demuxer.cc:204: Ranges<TimeDelta> GetBufferedRanges(base::TimeDelta duration) const; nit: perhaps ...
8 years, 4 months ago (2012-08-01 17:09:45 UTC) #2
vrk (LEFT CHROMIUM)
8 years, 4 months ago (2012-08-02 18:23:00 UTC) #3
https://chromiumcodereview.appspot.com/10829108/diff/3010/media/filters/chunk...
File media/filters/chunk_demuxer.cc (right):

https://chromiumcodereview.appspot.com/10829108/diff/3010/media/filters/chunk...
media/filters/chunk_demuxer.cc:204: Ranges<TimeDelta>
GetBufferedRanges(base::TimeDelta duration) const;
On 2012/08/01 17:09:45, acolwell wrote:
> nit: perhaps move this just below Append() so it's with similar methods called
> by ChunkDemuxer?

Done.

https://chromiumcodereview.appspot.com/10829108/diff/3010/media/filters/chunk...
media/filters/chunk_demuxer.cc:330: return
range.IntersectionWith(valid_time_range);
On 2012/08/01 17:09:45, acolwell wrote:
> \o/ look who's being tricky. I like it, but this deserves a comment.

haha glad you like! :) commented!

https://chromiumcodereview.appspot.com/10829108/diff/3010/media/filters/chunk...
media/filters/chunk_demuxer.cc:1145: Ranges<TimeDelta>
ChunkDemuxer::GetBufferedRanges() const {
On 2012/08/01 17:09:45, acolwell wrote:
> On 2012/08/01 00:10:06, Victoria Kirst wrote:
> > This CL adds this guy back again; if it's still overkill, I could make a
> method
> > to find specifically the last buffered timestamp.
> No this is fine here. My original complaint was it being overkill for the
append
> case. You're only calling this on eos now which is fine.
> 

SG!

https://chromiumcodereview.appspot.com/10829108/diff/3010/media/filters/chunk...
File media/filters/chunk_demuxer.h (right):

https://chromiumcodereview.appspot.com/10829108/diff/3010/media/filters/chunk...
media/filters/chunk_demuxer.h:147: // EoS is called.
On 2012/08/01 17:09:45, acolwell wrote:
> nit: Spell out EndOfStream() here. You've got the whole line . ;)

Done.

https://chromiumcodereview.appspot.com/10829108/diff/3010/media/filters/chunk...
File media/filters/chunk_demuxer_unittest.cc (right):

https://chromiumcodereview.appspot.com/10829108/diff/3010/media/filters/chunk...
media/filters/chunk_demuxer_unittest.cc:51: static const int
kDefaultSecondClusterDuration = 132;
On 2012/08/01 17:09:45, acolwell wrote:
> nit: Duration is misleading here. The cluster isn't 132ms long. It's 66ms.
> Perhaps s/Duration/EndTimestamp/ ?

Done.

https://chromiumcodereview.appspot.com/10829108/diff/3010/media/filters/chunk...
media/filters/chunk_demuxer_unittest.cc:2157: CheckExpectedRanges(kSourceId,
first_range.str());
On 2012/08/01 17:09:45, acolwell wrote:
> Just explicitly put the strings here and below. I'm pretty sure if these
values
> change other tests will break as well. You can add a comment above the call if
> you want to clarify where the numbers came from. I think that would be clearer
> than this.

Done, and I agree I think it's clearer this way.

Powered by Google App Engine
This is Rietveld 408576698