MSE: Parse WebM TrackEntry DefaultDuration field

MSE: Parse WebM TrackEntry DefaultDuration field

Adds WebM TrackEntry DefaultDuration parsing for use in later CLs.
This change is a prerequisite for defaulting a missing WebM frame's
duration to its track's DefaultDuration (if that optional field was
parsed), to reduce the scope of eventual missing frame duration
estimation.

BUG=351166
R=acolwell@chromium.org
TEST=All media_unittests and http/tests/media layout tests pass locally on Linux

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

[wolenetz, 2014-03-26 21:17:38]

PTAL. This is the first of several changes to get the all WebM frames to have
some duration, either from BlockGroup duration (current behavior), TrackEntry
DefaultDuration (if specified) for SimpleBlock duration (this change begins to
enable this) or estimation or default fallback (upcoming chromium change uses
this CL plus some estimation/default logic). After this and that next chromium
CL land, there will need to be some minor Blink layout test expectation updates
related to the new missing frame duration calculation/estimation logic.

[acolwell GONE FROM CHROMIUM, 2014-03-26 22:25:32]

lgtm % nits

https://codereview.chromium.org/213153008/diff/1/media/formats/webm/tracks_bu...
File media/formats/webm/tracks_builder.cc (right):

https://codereview.chromium.org/213153008/diff/1/media/formats/webm/tracks_bu...
media/formats/webm/tracks_builder.cc:92: for (int idx = 1; idx <= 8; ++idx) {
nit: How about SerializeInt(buf_ptr, buf_size_ptr, tmp.dst, 8) here instead to
avoid code duplication?

https://codereview.chromium.org/213153008/diff/1/media/formats/webm/tracks_bu...
media/formats/webm/tracks_builder.cc:226: int
TracksBuilder::Track::GetVideoPayloadSize() const {
nit: s/Payload/Element to match other naming here and elsewhere.

https://codereview.chromium.org/213153008/diff/1/media/formats/webm/tracks_bu...
media/formats/webm/tracks_builder.cc:237: int
TracksBuilder::Track::GetAudioPayloadSize() const {
ditto

https://codereview.chromium.org/213153008/diff/1/media/formats/webm/tracks_bu...
media/formats/webm/tracks_builder.cc:285: if (default_duration_ >= 0)
I wonder if we should have a "strict mode" that asserts if we try to use invalid
values. I can just see someone reusing this code to generate content "for realz"
and accidentally allowing this to be 0. Perhaps a TODO?

https://codereview.chromium.org/213153008/diff/1/media/formats/webm/webm_trac...
File media/formats/webm/webm_tracks_parser.cc (right):

https://codereview.chromium.org/213153008/diff/1/media/formats/webm/webm_trac...
media/formats/webm/webm_tracks_parser.cc:37: double mult = duration_in_ns /
1000.0;
nit: Do you really need the double here? Any fraction part from this division
will just get smaller on the following line and totally eliminated on the line
after that.

https://codereview.chromium.org/213153008/diff/1/media/formats/webm/webm_trac...
media/formats/webm/webm_tracks_parser.cc:40: if (mult == 0.0)
nit: if you don't do line 38 can't this just become mul < timecode_scale_in_us
and then you don't need the multiply below?

https://codereview.chromium.org/213153008/diff/1/media/formats/webm/webm_trac...
File media/formats/webm/webm_tracks_parser_unittest.cc (right):

https://codereview.chromium.org/213153008/diff/1/media/formats/webm/webm_trac...
media/formats/webm/webm_tracks_parser_unittest.cc:130: tb.AddTrack(1,
kWebMTrackTypeAudio, 1, "A_VORBIS", "audio", "", -1, -1, -1,
nit: Should we have AddVideoTrack(), AddAudioTrack(), and AddTextTrack() so
there are less -1's everywhere?

https://codereview.chromium.org/213153008/diff/1/media/formats/webm/webm_trac...
media/formats/webm/webm_tracks_parser_unittest.cc:141: EXPECT_EQ(kNoTimestamp(),
parser->GetAudioDefaultDuration(1000.0));
nit: make this a constant so it's a little clearer that this is a timecode scale

[wolenetz, 2014-03-27 01:21:43]

I'll send PS3 to CQ shortly. Thank you!

https://codereview.chromium.org/213153008/diff/1/media/formats/webm/tracks_bu...
File media/formats/webm/tracks_builder.cc (right):

https://codereview.chromium.org/213153008/diff/1/media/formats/webm/tracks_bu...
media/formats/webm/tracks_builder.cc:92: for (int idx = 1; idx <= 8; ++idx) {
On 2014/03/26 22:25:32, acolwell wrote:
> nit: How about SerializeInt(buf_ptr, buf_size_ptr, tmp.dst, 8) here instead to
> avoid code duplication?

Nice. Done.

https://codereview.chromium.org/213153008/diff/1/media/formats/webm/tracks_bu...
media/formats/webm/tracks_builder.cc:226: int
TracksBuilder::Track::GetVideoPayloadSize() const {
On 2014/03/26 22:25:32, acolwell wrote:
> nit: s/Payload/Element to match other naming here and elsewhere.

Hmm. I thought PayloadSize refers to the contents of the element, not include
the actual (master) element header, and (Master)ElementSize refers to the sum of
the (master) element header plus its payload size. See, for example, how there
is a distinct need for the payload vs the master sizes in:
Track::Write()'s WriteMasterElement(...kWebMIdAudio, GetAudioPayloadSize())
Track::GetPayloadSize()'s MasterElementSize(KWebMIdAudio,
GetAudioPayloadSize()).

Update: I confirmed offline my assumption that kWebMIdAudio and kWebMIdVideo
should use similar MasterElement and PayloadSize nomenclature like
kWebMIdTracks.

https://codereview.chromium.org/213153008/diff/1/media/formats/webm/tracks_bu...
media/formats/webm/tracks_builder.cc:237: int
TracksBuilder::Track::GetAudioPayloadSize() const {
On 2014/03/26 22:25:32, acolwell wrote:
> ditto

ditto (confirmed offline that s/Payload/Element/ is incorrect.)

https://codereview.chromium.org/213153008/diff/1/media/formats/webm/tracks_bu...
media/formats/webm/tracks_builder.cc:285: if (default_duration_ >= 0)
On 2014/03/26 22:25:32, acolwell wrote:
> I wonder if we should have a "strict mode" that asserts if we try to use
invalid
> values. I can just see someone reusing this code to generate content "for
realz"
> and accidentally allowing this to be 0. Perhaps a TODO?

Done (see new TracksBuilder |allow_invalid_values_|).
I also noted I forgot to add a test of parse failure if DefaultDuration is
specified as 0ns. I've added that unit test now.

https://codereview.chromium.org/213153008/diff/1/media/formats/webm/webm_trac...
File media/formats/webm/webm_tracks_parser.cc (right):

https://codereview.chromium.org/213153008/diff/1/media/formats/webm/webm_trac...
media/formats/webm/webm_tracks_parser.cc:37: double mult = duration_in_ns /
1000.0;
On 2014/03/26 22:25:32, acolwell wrote:
> nit: Do you really need the double here? Any fraction part from this division
> will just get smaller on the following line and totally eliminated on the line
> after that.

Say we have default webm timecode scale_in_us of 1000.0, and a common
duration_in_ns corresponding to 29.97 fps = 33366700. Capped precision of this
default duration should then be 33ms. Let's work through the proposed
calculation:
int64 mult = 33366700 / 1000; (= 33366)
// mult is not less than 1000, so we don't return kNoTimestamp();
But, we return a default duration TimeDelta that has too much precision
(33.366ms, not 33ms).

So I think using int64 and dropping the std::floor() is ok, but we need the
division + multiplication to enforce the precision cap.

https://codereview.chromium.org/213153008/diff/1/media/formats/webm/webm_trac...
media/formats/webm/webm_tracks_parser.cc:40: if (mult == 0.0)
On 2014/03/26 22:25:32, acolwell wrote:
> nit: if you don't do line 38 can't this just become mul < timecode_scale_in_us
> and then you don't need the multiply below?

I don't think so. See previous comment's reply.

https://codereview.chromium.org/213153008/diff/1/media/formats/webm/webm_trac...
File media/formats/webm/webm_tracks_parser_unittest.cc (right):

https://codereview.chromium.org/213153008/diff/1/media/formats/webm/webm_trac...
media/formats/webm/webm_tracks_parser_unittest.cc:130: tb.AddTrack(1,
kWebMTrackTypeAudio, 1, "A_VORBIS", "audio", "", -1, -1, -1,
On 2014/03/26 22:25:32, acolwell wrote:
> nit: Should we have AddVideoTrack(), AddAudioTrack(), and AddTextTrack() so
> there are less -1's everywhere?

Done.

https://codereview.chromium.org/213153008/diff/1/media/formats/webm/webm_trac...
media/formats/webm/webm_tracks_parser_unittest.cc:141: EXPECT_EQ(kNoTimestamp(),
parser->GetAudioDefaultDuration(1000.0));
On 2014/03/26 22:25:32, acolwell wrote:
> nit: make this a constant so it's a little clearer that this is a timecode
scale

Done.

[wolenetz, 2014-03-27 01:21:49]

The CQ bit was checked by wolenetz@chromium.org

[commit-bot: I haz the power, 2014-03-27 01:24:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wolenetz@chromium.org/213153008/30001

[commit-bot: I haz the power, 2014-03-27 07:46:35]

Change committed as 259815