Update media duration if data is appended after the previous duration

Update media duration if data is appended after the previous duration

BUG=139044
TEST=media_unittests

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=149651

[vrk (LEFT CHROMIUM), 2012-07-19 19:15:42]

This is another not-to-be-committed CL based on
https://chromiumcodereview.appspot.com/10803019/

https://chromiumcodereview.appspot.com/10800041/diff/1/media/base/data_source.h
File media/base/data_source.h (left):

https://chromiumcodereview.appspot.com/10800041/diff/1/media/base/data_source...
media/base/data_source.h:26: virtual void AddBufferedTimeRange(base::TimeDelta
start,
(this change was not necessary; just a clean-up move)

[acolwell GONE FROM CHROMIUM, 2012-07-20 21:18:36]

https://chromiumcodereview.appspot.com/10800041/diff/3001/media/filters/chunk...
File media/filters/chunk_demuxer.cc (left):

https://chromiumcodereview.appspot.com/10800041/diff/3001/media/filters/chunk...
media/filters/chunk_demuxer.cc:943: host_->SetDuration(duration_);
This isn't needed anymore because of the code in OnNewMediaSegment()?

https://chromiumcodereview.appspot.com/10800041/diff/3001/media/filters/chunk...
File media/filters/chunk_demuxer.cc (right):

https://chromiumcodereview.appspot.com/10800041/diff/3001/media/filters/chunk...
media/filters/chunk_demuxer.cc:1068: duration_ = std::max(duration_,
start_time_);
A couple questions here:
- Shouldn't the SetDuration() call be conditional on whether duration_ changed
or not? 
- Don't we want to use (start_time - start_time_) here for a duration? If the
first append starts at 10 seconds I don't think we want the duration to be 10
seconds.
- WDYT about creating a UpdateDuration() helper function so duration_ updates
and host_->SetDuration() calls are all in one place?

[vrk (LEFT CHROMIUM), 2012-07-26 22:27:26]

Will add tests next! I described some behavior decisions in the (code review)
comments -- let me know if you think I should do things differently.

https://chromiumcodereview.appspot.com/10800041/diff/3001/media/filters/chunk...
File media/filters/chunk_demuxer.cc (left):

https://chromiumcodereview.appspot.com/10800041/diff/3001/media/filters/chunk...
media/filters/chunk_demuxer.cc:943: host_->SetDuration(duration_);
On 2012/07/20 21:18:36, acolwell wrote:
> This isn't needed anymore because of the code in OnNewMediaSegment()?

Yes. My thought was, I was moving this line to happen right before transitioning
to the INITIALIZED state, which is where it happened prior to my change that
added WAITING_FOR_START_TIME. However, there's no harm in keeping this here, and
that means I don't have to change unit tests :) so I reinstated these lines!

https://chromiumcodereview.appspot.com/10800041/diff/3001/media/filters/chunk...
File media/filters/chunk_demuxer.cc (right):

https://chromiumcodereview.appspot.com/10800041/diff/3001/media/filters/chunk...
media/filters/chunk_demuxer.cc:1068: duration_ = std::max(duration_,
start_time_);
On 2012/07/20 21:18:36, acolwell wrote:
> A couple questions here:
> - Shouldn't the SetDuration() call be conditional on whether duration_ changed
> or not? 

As I said in my other comment, my thinking was I was moving the SetDuration()
call from OnStreamParserInitDone() to here.

But adding a conditional makes sense for 2 reasons: a) unittests require fewer
changes, b) Append()s may occur before we call the init_cb_, and these Append()s
may increase the duration.

> - Don't we want to use (start_time - start_time_) here for a duration? If the
> first append starts at 10 seconds I don't think we want the duration to be 10
> seconds.
I think we run into weirdness when the duration is less than the current time
(i.e. a lot of the code assumes duration == end of file). See bug:
crbug.com/137275

It is unideal/unintuitive logic - clearly the duration is going to be >
start_time_, since start_time_ is where it starts! But I need to set a more
accurate duration before calling the |init_cb_|, and if start_time_ > duration_,
then I believe start_time_ is the most accurate duration we have.

> - WDYT about creating a UpdateDuration() helper function so duration_ updates
> and host_->SetDuration() calls are all in one place?

SG! Done.

https://chromiumcodereview.appspot.com/10800041/diff/6001/media/filters/pipel...
File media/filters/pipeline_integration_test.cc (right):

https://chromiumcodereview.appspot.com/10800041/diff/6001/media/filters/pipel...
media/filters/pipeline_integration_test.cc:195: null_cb);
Added these as null cbs because PipelineIntegrationTest shouldn't notify WebKit
about duration changes.

https://chromiumcodereview.appspot.com/10800041/diff/6005/media/filters/chunk...
File media/filters/chunk_demuxer.cc (right):

https://chromiumcodereview.appspot.com/10800041/diff/6005/media/filters/chunk...
media/filters/chunk_demuxer.cc:1131: const StreamParser::BufferQueue& buffers) {
This function was added to fix that problem we were discussing yesterday, where
the fudge room would push the last buffer over |duration_|, even though it's the
last buffer before end of stream. The way I'm fixing the problem is I'm only
updating the duration_ if the buffer's timestamp is greater than duration_, but
I use the buffered time (i.e. timestamp+fudge) as the value for the |duration_|
... if that makes sense!

[acolwell GONE FROM CHROMIUM, 2012-07-27 00:03:22]

Looks pretty good. I'm assuming explicit duration setting from JavaScript will
be in a different CL?

https://chromiumcodereview.appspot.com/10800041/diff/17001/media/base/clock.cc
File media/base/clock.cc (left):

https://chromiumcodereview.appspot.com/10800041/diff/17001/media/base/clock.c...
media/base/clock.cc:85: duration_ = duration;
I think you need the following
media_time_ = ClampToValidTimeRange(media_time_);
max_time_ = ClampToValidTimeRange(max_time_);

https://chromiumcodereview.appspot.com/10800041/diff/17001/media/base/demuxer.h
File media/base/demuxer.h (right):

https://chromiumcodereview.appspot.com/10800041/diff/17001/media/base/demuxer...
media/base/demuxer.h:17: class MEDIA_EXPORT DemuxerHost : public DataSourceHost
{
Since AddBufferedTimeRange() is in here now, can we remove the DataSourceHost
inheritance? If so, but it triggers a bunch of changes. Consider peeling this
off into a separate CL.

https://chromiumcodereview.appspot.com/10800041/diff/17001/media/base/pipelin...
File media/base/pipeline.cc (right):

https://chromiumcodereview.appspot.com/10800041/diff/17001/media/base/pipelin...
media/base/pipeline.cc:426: old_duration = clock_->Duration();
nit: How about just having a duration_changed bool to control when the
new_duration_cb_ is called and only calling SetDuration() when the duration has
changed? Then SetDuration() & the callback are called under the same
circumstances which seems more consistent to me.

https://chromiumcodereview.appspot.com/10800041/diff/17001/media/base/pipelin...
media/base/pipeline.cc:592: const base::Closure& new_duration_cb) {
WDYT about just having WebMediaPlayerImpl or even HTMLMediaElement call
duration() before and after a sourceAppend() or sourceEndOfStream() to check for
duration changes? Could that work? I'm just worried about JavaScript visible
inconsistancy between duration, HTMLMediaElement.buffered, and
SourceBuffer.buffered since this callback requires a thread hop. If no
inconsistency is visible then I'm fine with the callback.

https://chromiumcodereview.appspot.com/10800041/diff/17001/media/filters/chun...
File media/filters/chunk_demuxer.cc (right):

https://chromiumcodereview.appspot.com/10800041/diff/17001/media/filters/chun...
media/filters/chunk_demuxer.cc:1092: UpdateDuration(start_time_);
nit: Could this be up where start_time_ is initially set?
Should we have a comment here indicating that this should be changed to
(start_time - start_time_) once http://crbug.com/137275 is addressed?

https://chromiumcodereview.appspot.com/10800041/diff/17001/media/filters/chun...
media/filters/chunk_demuxer.cc:1125: duration_ = new_duration;
DCHECK(duration_ != new_duration) to make sure code only notifies on changes?

https://chromiumcodereview.appspot.com/10800041/diff/17001/media/filters/chun...
media/filters/chunk_demuxer.cc:1126: host_->SetDuration(new_duration);
Does this cause double duration notifications on muxed content appended to the
end? If so I wonder if we should defer notifying the host until the end of
AppendData() & EndOfStream()?

https://chromiumcodereview.appspot.com/10800041/diff/17001/media/filters/chun...
media/filters/chunk_demuxer.cc:1137: DCHECK(last_timestamp_buffered >
duration_);
I don't think this is right. The buffers aren't appended yet so they won't be
reflected by GetBufferedRanges(). Also calling GetBufferedRanges() seems a
little heavy-weight here especially since the caller knows whether the buffers
are being appended to audio_ or video_. You could just pass a ChunkDemuxerStream
pointer to this method and call GetBufferedRanges() on that.

https://chromiumcodereview.appspot.com/10800041/diff/17001/media/filters/chun...
File media/filters/chunk_demuxer.h (right):

https://chromiumcodereview.appspot.com/10800041/diff/17001/media/filters/chun...
media/filters/chunk_demuxer.h:139: // Increases |duration_| if the newly
appended |buffers| exceeds the current
nit: s/exceeds/exceed/?

[vrk (LEFT CHROMIUM), 2012-07-28 01:42:05]

Thanks Aaron! Responded to comments and added a couple unit tests.

> Looks pretty good. I'm assuming explicit duration setting from JavaScript will
> be in a different CL?

Yes, as will truncating the duration on end of stream! Need to talk to you about
the end of stream case.

https://chromiumcodereview.appspot.com/10800041/diff/17001/media/base/clock.cc
File media/base/clock.cc (left):

https://chromiumcodereview.appspot.com/10800041/diff/17001/media/base/clock.c...
media/base/clock.cc:85: duration_ = duration;
On 2012/07/27 00:03:22, acolwell wrote:
> I think you need the following
> media_time_ = ClampToValidTimeRange(media_time_);
> max_time_ = ClampToValidTimeRange(max_time_);

Good point! Done.

https://chromiumcodereview.appspot.com/10800041/diff/17001/media/base/demuxer.h
File media/base/demuxer.h (right):

https://chromiumcodereview.appspot.com/10800041/diff/17001/media/base/demuxer...
media/base/demuxer.h:17: class MEDIA_EXPORT DemuxerHost : public DataSourceHost
{
On 2012/07/27 00:03:23, acolwell wrote:
> Since AddBufferedTimeRange() is in here now, can we remove the DataSourceHost
> inheritance? 
Possibly, I need to chat with you about it though.

> If so, but it triggers a bunch of changes. Consider peeling this
> off into a separate CL.

Reverted the files for now.

https://chromiumcodereview.appspot.com/10800041/diff/17001/media/base/pipelin...
File media/base/pipeline.cc (right):

https://chromiumcodereview.appspot.com/10800041/diff/17001/media/base/pipelin...
media/base/pipeline.cc:426: old_duration = clock_->Duration();
On 2012/07/27 00:03:23, acolwell wrote:
> nit: How about just having a duration_changed bool to control when the
> new_duration_cb_ is called and only calling SetDuration() when the duration
has
> changed? Then SetDuration() & the callback are called under the same
> circumstances which seems more consistent to me.

No longer necessary after getting rid of new_duration_cb_.

https://chromiumcodereview.appspot.com/10800041/diff/17001/media/base/pipelin...
media/base/pipeline.cc:592: const base::Closure& new_duration_cb) {
On 2012/07/27 00:03:23, acolwell wrote:
> WDYT about just having WebMediaPlayerImpl or even HTMLMediaElement call
> duration() before and after a sourceAppend() or sourceEndOfStream() to check
for
> duration changes? Could that work? I'm just worried about JavaScript visible
> inconsistancy between duration, HTMLMediaElement.buffered, and
> SourceBuffer.buffered since this callback requires a thread hop. If no
> inconsistency is visible then I'm fine with the callback.

Done (in WebMediaPlayerImpl) and I agree, this is superior! I didn't feel good
about adding this callback so I'm glad I can remove it!

https://chromiumcodereview.appspot.com/10800041/diff/17001/media/filters/chun...
File media/filters/chunk_demuxer.cc (right):

https://chromiumcodereview.appspot.com/10800041/diff/17001/media/filters/chun...
media/filters/chunk_demuxer.cc:1092: UpdateDuration(start_time_);
On 2012/07/27 00:03:23, acolwell wrote:
> nit: Could this be up where start_time_ is initially set?
> Should we have a comment here indicating that this should be changed to
> (start_time - start_time_) once http://crbug.com/137275 is addressed?

Done.

https://chromiumcodereview.appspot.com/10800041/diff/17001/media/filters/chun...
media/filters/chunk_demuxer.cc:1125: duration_ = new_duration;
On 2012/07/27 00:03:23, acolwell wrote:
> DCHECK(duration_ != new_duration) to make sure code only notifies on changes?

Done.

https://chromiumcodereview.appspot.com/10800041/diff/17001/media/filters/chun...
media/filters/chunk_demuxer.cc:1126: host_->SetDuration(new_duration);
On 2012/07/27 00:03:23, acolwell wrote:
> Does this cause double duration notifications on muxed content appended to the
> end?

Yes this happens!

> If so I wonder if we should defer notifying the host until the end of
> AppendData() & EndOfStream()?

Talked offline; it's a little icky to do this elegantly so I'll keep as-is for
now!

https://chromiumcodereview.appspot.com/10800041/diff/17001/media/filters/chun...
media/filters/chunk_demuxer.cc:1137: DCHECK(last_timestamp_buffered >
duration_);
On 2012/07/27 00:03:23, acolwell wrote:
> I don't think this is right. The buffers aren't appended yet so they won't be
> reflected by GetBufferedRanges().

AHH you are right! I must have fiddled with this before testing, this breaks
pretty immediately when tried out :) 

Fixed, and the duration change tests I added also fail with this un-fixed.

> Also calling GetBufferedRanges() seems a
> little heavy-weight here especially since the caller knows whether the buffers
> are being appended to audio_ or video_. You could just pass a
ChunkDemuxerStream
> pointer to this method and call GetBufferedRanges() on that.

Done, but FYI this changes the behavior a little bit. If in a muxed stream, the
audio data ends at 25s and the video ends at 30s  (and the original duration
was, say, 20s), the code here would say the duration is 25s (which matches
sourceBuffered()), while your suggested changes makes the duration 30s. I think
30s makes more sense, though it would mean that the duration would always exceed
sourceBuffered() if the audio and video don't line up perfectly at the end. Do
you have thoughts on this?

https://chromiumcodereview.appspot.com/10800041/diff/17001/media/filters/chun...
File media/filters/chunk_demuxer.h (right):

https://chromiumcodereview.appspot.com/10800041/diff/17001/media/filters/chun...
media/filters/chunk_demuxer.h:139: // Increases |duration_| if the newly
appended |buffers| exceeds the current
On 2012/07/27 00:03:23, acolwell wrote:
> nit: s/exceeds/exceed/?

Done.

https://chromiumcodereview.appspot.com/10800041/diff/16003/media/filters/chun...
File media/filters/chunk_demuxer_unittest.cc (right):

https://chromiumcodereview.appspot.com/10800041/diff/16003/media/filters/chun...
media/filters/chunk_demuxer_unittest.cc:2148: kAudioBlockDuration * 2)));
These values here and below are a bit weird... probably best if I explain in
person on Monday :)

[acolwell GONE FROM CHROMIUM, 2012-07-30 21:37:45]

LGTM

https://chromiumcodereview.appspot.com/10800041/diff/16003/media/base/pipeline.h
File media/base/pipeline.h (right):

https://chromiumcodereview.appspot.com/10800041/diff/16003/media/base/pipelin...
media/base/pipeline.h:296: virtual void AddBufferedTimeRange(base::TimeDelta
start,
nit: Revert this too?

https://chromiumcodereview.appspot.com/10800041/diff/16003/media/filters/chun...
File media/filters/chunk_demuxer.cc (right):

https://chromiumcodereview.appspot.com/10800041/diff/16003/media/filters/chun...
media/filters/chunk_demuxer.cc:939: if (duration > duration_)
I believe the new spec text specifies that only the first init segment w/ a
duration should effect duration_. Change this to duration != base::TimeDelta()?

https://chromiumcodereview.appspot.com/10800041/diff/16003/media/filters/chun...
media/filters/chunk_demuxer.cc:1077: 
Based on the spec text, I believe if we reach here and duration_ still equals
base::TimeDelta() then it should get set to kInfiniteDuration().

[vrk (LEFT CHROMIUM), 2012-07-31 22:39:01]

Thanks for reviewing!

https://chromiumcodereview.appspot.com/10800041/diff/16003/media/base/pipeline.h
File media/base/pipeline.h (right):

https://chromiumcodereview.appspot.com/10800041/diff/16003/media/base/pipelin...
media/base/pipeline.h:296: virtual void AddBufferedTimeRange(base::TimeDelta
start,
On 2012/07/30 21:37:45, acolwell wrote:
> nit: Revert this too?

Done.

https://chromiumcodereview.appspot.com/10800041/diff/16003/media/filters/chun...
File media/filters/chunk_demuxer.cc (right):

https://chromiumcodereview.appspot.com/10800041/diff/16003/media/filters/chun...
media/filters/chunk_demuxer.cc:939: if (duration > duration_)
On 2012/07/30 21:37:45, acolwell wrote:
> I believe the new spec text specifies that only the first init segment w/ a
> duration should effect duration_. Change this to duration !=
base::TimeDelta()?

Changed to if (duration != base::TimeDelta() && duration_ == base::TimeDelta()).

https://chromiumcodereview.appspot.com/10800041/diff/16003/media/filters/chun...
media/filters/chunk_demuxer.cc:1077: 
On 2012/07/30 21:37:45, acolwell wrote:
> Based on the spec text, I believe if we reach here and duration_ still equals
> base::TimeDelta() then it should get set to kInfiniteDuration().

Done.