Chrome-side implementation of media source timestamp offset

Chrome-side implementation of media source timestamp offset

Adds functionality to signal an offset to be applied to the
buffers in ChunkDemuxer. Is not triggerable from Chrome yet.

BUG=139044
TEST=media_unittests

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

[vrk (LEFT CHROMIUM), 2012-07-18 20:37:01]

Here's what I did to implement the timestamp offset. There are many other ways
to do this, so let me know if you want me to try another approach!

Also apply patch at https://bugs.webkit.org/show_bug.cgi?id=91662

https://chromiumcodereview.appspot.com/10803019/diff/1/media/mp4/mp4_stream_p...
File media/mp4/mp4_stream_parser.cc (right):

https://chromiumcodereview.appspot.com/10803019/diff/1/media/mp4/mp4_stream_p...
media/mp4/mp4_stream_parser.cc:111: // TODO(someone else): Implement.
I could either implement these or set an owner for the TODO :)

[acolwell GONE FROM CHROMIUM, 2012-07-18 22:54:55]

How hard would this be to implement this in ChunkDemuxer only so that you
wouldn't need to modify each of the parsers?

WDYT of removing clearTimestampOffset() and just using setTimestampOffset(0)?

On 2012/07/18 20:37:01, Victoria Kirst wrote:
> Here's what I did to implement the timestamp offset. There are many other ways
> to do this, so let me know if you want me to try another approach!
> 
> Also apply patch at https://bugs.webkit.org/show_bug.cgi?id=91662
> 
>
https://chromiumcodereview.appspot.com/10803019/diff/1/media/mp4/mp4_stream_p...
> File media/mp4/mp4_stream_parser.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10803019/diff/1/media/mp4/mp4_stream_p...
> media/mp4/mp4_stream_parser.cc:111: // TODO(someone else): Implement.
> I could either implement these or set an owner for the TODO :)

[vrk (LEFT CHROMIUM), 2012-07-20 01:56:36]

On 2012/07/18 22:54:55, acolwell wrote:
> How hard would this be to implement this in ChunkDemuxer only so that you
> wouldn't need to modify each of the parsers?

Done! Still not a fan of the maps in ChunkDemuxer (decided against merging into
one map because I thought stream_info_map_[id].parser->Parse() was more
confusing) but I think moving the logic into ChunkDemuxer is the right thing to
do. Let me know what you think.

> WDYT of removing clearTimestampOffset() and just using setTimestampOffset(0)?

Agreed, the JS I wrote to test this doesn't use clearTimestampOffset(), which
says something :)

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

This looks good to me with a few minor nits. If you add a test or two I think we
should check it in.

https://chromiumcodereview.appspot.com/10803019/diff/16/media/filters/chunk_d...
File media/filters/chunk_demuxer.cc (right):

https://chromiumcodereview.appspot.com/10803019/diff/16/media/filters/chunk_d...
media/filters/chunk_demuxer.cc:1002:
stream_info_map_[source_id_video_].can_update_offset = false;
did you mean source_id_audio_ here?

https://chromiumcodereview.appspot.com/10803019/diff/16/media/filters/chunk_d...
media/filters/chunk_demuxer.cc:1017:
stream_info_map_[source_id_video_].can_update_offset = false;
Perhaps move this to OnNewMediaSegment()? That way it is symmetrical with
OnEndOfMediaSegment().

https://chromiumcodereview.appspot.com/10803019/diff/16/media/filters/chunk_d...
File media/filters/chunk_demuxer.h (right):

https://chromiumcodereview.appspot.com/10803019/diff/16/media/filters/chunk_d...
media/filters/chunk_demuxer.h:80: bool TimestampOffset(const std::string& id,
float offset);
nit: SetTimestampOffset()?

https://chromiumcodereview.appspot.com/10803019/diff/16/media/filters/chunk_d...
media/filters/chunk_demuxer.h:152: struct StreamInfo {
nit: s/StreamInfo/SourceInfo or SourceIdInfo since this is source buffer
specific not stream specific?

https://chromiumcodereview.appspot.com/10803019/diff/16/media/mp4/mp4_stream_...
File media/mp4/mp4_stream_parser.cc (right):

https://chromiumcodereview.appspot.com/10803019/diff/16/media/mp4/mp4_stream_...
media/mp4/mp4_stream_parser.cc:247: ChangeState(kParsingBoxes);
I believe this is where you should call end_of_segment_cb_.Run()

https://chromiumcodereview.appspot.com/10803019/diff/16/media/webm/webm_clust...
File media/webm/webm_cluster_parser.cc (right):

https://chromiumcodereview.appspot.com/10803019/diff/16/media/webm/webm_clust...
media/webm/webm_cluster_parser.cc:43: cluster_start_time_ = kNoTimestamp();
cluster_ended_ = false

https://chromiumcodereview.appspot.com/10803019/diff/16/media/webm/webm_clust...
File media/webm/webm_cluster_parser.h (right):

https://chromiumcodereview.appspot.com/10803019/diff/16/media/webm/webm_clust...
media/webm/webm_cluster_parser.h:42: bool cluster_ended() const { return
cluster_ended_; }
Add a comment indicating this returns true if the last Parse() call stopped at
the end of a cluster.

[vrk (LEFT CHROMIUM), 2012-07-25 17:16:48]

Thanks Aaron, PTAL! I added some unit tests.

https://chromiumcodereview.appspot.com/10803019/diff/16/media/filters/chunk_d...
File media/filters/chunk_demuxer.cc (right):

https://chromiumcodereview.appspot.com/10803019/diff/16/media/filters/chunk_d...
media/filters/chunk_demuxer.cc:1002:
stream_info_map_[source_id_video_].can_update_offset = false;
On 2012/07/20 18:21:01, acolwell wrote:
> did you mean source_id_audio_ here?

d'oh. Yes :) My test page only uses video, so I didn't catch this!

I actually removed this line in response to the comment below, but the unit
tests I added catches the equivalent mistake.

https://chromiumcodereview.appspot.com/10803019/diff/16/media/filters/chunk_d...
media/filters/chunk_demuxer.cc:1017:
stream_info_map_[source_id_video_].can_update_offset = false;
On 2012/07/20 18:21:01, acolwell wrote:
> Perhaps move this to OnNewMediaSegment()? That way it is symmetrical with
> OnEndOfMediaSegment().

Done and removed from OnAudioBuffers() as well.

https://chromiumcodereview.appspot.com/10803019/diff/16/media/filters/chunk_d...
File media/filters/chunk_demuxer.h (right):

https://chromiumcodereview.appspot.com/10803019/diff/16/media/filters/chunk_d...
media/filters/chunk_demuxer.h:80: bool TimestampOffset(const std::string& id,
float offset);
On 2012/07/20 18:21:01, acolwell wrote:
> nit: SetTimestampOffset()?

Done, and changed other similar methods.

https://chromiumcodereview.appspot.com/10803019/diff/16/media/filters/chunk_d...
media/filters/chunk_demuxer.h:152: struct StreamInfo {
On 2012/07/20 18:21:01, acolwell wrote:
> nit: s/StreamInfo/SourceInfo or SourceIdInfo since this is source buffer
> specific not stream specific?

Done.

https://chromiumcodereview.appspot.com/10803019/diff/16/media/mp4/mp4_stream_...
File media/mp4/mp4_stream_parser.cc (right):

https://chromiumcodereview.appspot.com/10803019/diff/16/media/mp4/mp4_stream_...
media/mp4/mp4_stream_parser.cc:247: ChangeState(kParsingBoxes);
On 2012/07/20 18:21:01, acolwell wrote:
> I believe this is where you should call end_of_segment_cb_.Run()

Done, and fixed some indentation nits since I was in the neighborhood.

https://chromiumcodereview.appspot.com/10803019/diff/16/media/webm/webm_clust...
File media/webm/webm_cluster_parser.cc (right):

https://chromiumcodereview.appspot.com/10803019/diff/16/media/webm/webm_clust...
media/webm/webm_cluster_parser.cc:43: cluster_start_time_ = kNoTimestamp();
On 2012/07/20 18:21:01, acolwell wrote:
> cluster_ended_ = false

Done.

https://chromiumcodereview.appspot.com/10803019/diff/16/media/webm/webm_clust...
File media/webm/webm_cluster_parser.h (right):

https://chromiumcodereview.appspot.com/10803019/diff/16/media/webm/webm_clust...
media/webm/webm_cluster_parser.h:42: bool cluster_ended() const { return
cluster_ended_; }
On 2012/07/20 18:21:01, acolwell wrote:
> Add a comment indicating this returns true if the last Parse() call stopped at
> the end of a cluster.

Done.

[acolwell GONE FROM CHROMIUM, 2012-07-25 18:22:43]

LGTM % nits and don't forget to change issue title & description.

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

https://chromiumcodereview.appspot.com/10803019/diff/17003/media/filters/chun...
media/filters/chunk_demuxer.cc:818: DVLOG(1) << "TimestampOffset(" << id << ", "
<< offset << ")";
nit: Add Set

https://chromiumcodereview.appspot.com/10803019/diff/17003/media/filters/chun...
media/filters/chunk_demuxer.cc:824: if (!source_info_map_[id].can_update_offset)
nit: move this above time_offset since we don't need to compute this if we can't
update the offset.

https://chromiumcodereview.appspot.com/10803019/diff/17003/media/filters/chun...
media/filters/chunk_demuxer.cc:1068: CHECK_GT(source_info_map_.count(source_id),
0u);
nit: Create a bool IsValidId(source_id) helper so we don't have this count()
construct everywhere?

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

https://chromiumcodereview.appspot.com/10803019/diff/17003/media/filters/chun...
media/filters/chunk_demuxer.h:25: class SourceInfo;
nit: I don't think you need this anymore since it is declared below.

https://chromiumcodereview.appspot.com/10803019/diff/17003/media/filters/chun...
media/filters/chunk_demuxer.h:80: bool SetTimestampOffset(const std::string& id,
float offset);
Document return value. I'm assuming returning false means the offset wasn't set
because we are in the middle of parsing a media segment.

https://chromiumcodereview.appspot.com/10803019/diff/17003/media/filters/chun...
media/filters/chunk_demuxer.h:151: // Contains state belonging to a media
source.
s/media source/source ID?

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

https://chromiumcodereview.appspot.com/10803019/diff/17003/media/filters/chun...
media/filters/chunk_demuxer_unittest.cc:1807:
demuxer_->SetTimestampOffset(kSourceId, 30);
nit: ASSERT_TRUE() here and below.

https://chromiumcodereview.appspot.com/10803019/diff/17003/media/filters/chun...
media/filters/chunk_demuxer_unittest.cc:1818:
audio->Read(base::Bind(&OnReadDone,
nit: I think using GenerateExpectedReads(30000, 2, audio, video) would work and
be fewer lines.

https://chromiumcodereview.appspot.com/10803019/diff/17003/media/filters/chun...
media/filters/chunk_demuxer_unittest.cc:1843:
audio->Read(base::Bind(&OnReadDone,
nit: ditto w/ different timestamp

https://chromiumcodereview.appspot.com/10803019/diff/17003/media/filters/chun...
media/filters/chunk_demuxer_unittest.cc:1881: }  // namespace media
nit: Add a test for rejecting an offset change mid-segment.

[vrk (LEFT CHROMIUM), 2012-07-25 22:56:49]

Thanks for reviewing!

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

https://chromiumcodereview.appspot.com/10803019/diff/17003/media/filters/chun...
media/filters/chunk_demuxer.cc:818: DVLOG(1) << "TimestampOffset(" << id << ", "
<< offset << ")";
On 2012/07/25 18:22:43, acolwell wrote:
> nit: Add Set

Done.

https://chromiumcodereview.appspot.com/10803019/diff/17003/media/filters/chun...
media/filters/chunk_demuxer.cc:824: if (!source_info_map_[id].can_update_offset)
On 2012/07/25 18:22:43, acolwell wrote:
> nit: move this above time_offset since we don't need to compute this if we
can't
> update the offset.

Done.

https://chromiumcodereview.appspot.com/10803019/diff/17003/media/filters/chun...
media/filters/chunk_demuxer.cc:1068: CHECK_GT(source_info_map_.count(source_id),
0u);
On 2012/07/25 18:22:43, acolwell wrote:
> nit: Create a bool IsValidId(source_id) helper so we don't have this count()
> construct everywhere?

Done.

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

https://chromiumcodereview.appspot.com/10803019/diff/17003/media/filters/chun...
media/filters/chunk_demuxer.h:25: class SourceInfo;
On 2012/07/25 18:22:43, acolwell wrote:
> nit: I don't think you need this anymore since it is declared below.

Done.

https://chromiumcodereview.appspot.com/10803019/diff/17003/media/filters/chun...
media/filters/chunk_demuxer.h:80: bool SetTimestampOffset(const std::string& id,
float offset);
On 2012/07/25 18:22:43, acolwell wrote:
> Document return value. I'm assuming returning false means the offset wasn't
set
> because we are in the middle of parsing a media segment.

Done.

https://chromiumcodereview.appspot.com/10803019/diff/17003/media/filters/chun...
media/filters/chunk_demuxer.h:151: // Contains state belonging to a media
source.
On 2012/07/25 18:22:43, acolwell wrote:
> s/media source/source ID?

yeah that sounds a bit better, changed!

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

https://chromiumcodereview.appspot.com/10803019/diff/17003/media/filters/chun...
media/filters/chunk_demuxer_unittest.cc:1807:
demuxer_->SetTimestampOffset(kSourceId, 30);
On 2012/07/25 18:22:43, acolwell wrote:
> nit: ASSERT_TRUE() here and below.

Done.

https://chromiumcodereview.appspot.com/10803019/diff/17003/media/filters/chun...
media/filters/chunk_demuxer_unittest.cc:1818:
audio->Read(base::Bind(&OnReadDone,
On 2012/07/25 18:22:43, acolwell wrote:
> nit: I think using GenerateExpectedReads(30000, 2, audio, video) would work
and
> be fewer lines.

Done.

https://chromiumcodereview.appspot.com/10803019/diff/17003/media/filters/chun...
media/filters/chunk_demuxer_unittest.cc:1843:
audio->Read(base::Bind(&OnReadDone,
On 2012/07/25 18:22:43, acolwell wrote:
> nit: ditto w/ different timestamp

Done.

https://chromiumcodereview.appspot.com/10803019/diff/17003/media/filters/chun...
media/filters/chunk_demuxer_unittest.cc:1881: }  // namespace media
On 2012/07/25 18:22:43, acolwell wrote:
> nit: Add a test for rejecting an offset change mid-segment.

Good idea, done.