Implement reading of media track info from WebM and MP4 containers

Implement reading of media track info from WebM and MP4 containers

BUG=590085
Committed: https://crrev.com/d307b5f1d4fe75cb8a3d1e080d4890ae757d1634
Cr-Commit-Position: refs/heads/master@{#380532}

[servolk, 2016-03-01 04:02:52]

Description was changed from

==========
Implemented sourcing media track info from webm/mp4 containers for MSE

BUG=590085
==========

to

==========
Implemented reading media track info from webm/mp4 containers

BUG=590085
==========

[servolk, 2016-03-01 04:02:53]

servolk@chromium.org changed reviewers:
+ wolenetz@chromium.org

[ddorwin, 2016-03-01 17:05:12]

ddorwin@chromium.org changed reviewers:
+ ddorwin@chromium.org

[ddorwin, 2016-03-01 17:05:13]

Commit message nit:
Implement reading of media track info from WebM and MP4 containers

[ddorwin, 2016-03-01 17:05:29]

ddorwin@chromium.org changed reviewers:
- ddorwin@chromium.org

[servolk, 2016-03-01 17:14:20]

Description was changed from

==========
Implemented reading media track info from webm/mp4 containers

BUG=590085
==========

to

==========
Implement reading media track info from webm/mp4 containers

BUG=590085
==========

[servolk, 2016-03-01 18:31:40]

On 2016/03/01 17:05:13, ddorwin wrote:
> Commit message nit:
> Implement reading of media track info from WebM and MP4 containers

Done

[ddorwin, 2016-03-01 19:05:40]

On 2016/03/01 18:31:40, servolk wrote:
> On 2016/03/01 17:05:13, ddorwin wrote:
> > Commit message nit:
> > Implement reading of media track info from WebM and MP4 containers
> 
> Done

Note the "of", "and", and casing.
I'm not super concerned about the "of" - I was trying to make that read better.
However, the '/' should be replaced and correct casing is preferred.

[servolk, 2016-03-01 19:16:33]

Description was changed from

==========
Implement reading media track info from webm/mp4 containers

BUG=590085
==========

to

==========
Implement reading of media track info from WebM and MP4 containers

BUG=590085
==========

[servolk, 2016-03-01 19:16:47]

On 2016/03/01 19:05:40, ddorwin wrote:
> On 2016/03/01 18:31:40, servolk wrote:
> > On 2016/03/01 17:05:13, ddorwin wrote:
> > > Commit message nit:
> > > Implement reading of media track info from WebM and MP4 containers
> > 
> > Done
> 
> Note the "of", "and", and casing.
> I'm not super concerned about the "of" - I was trying to make that read
better.
> However, the '/' should be replaced and correct casing is preferred.

Done

[wolenetz, 2016-03-05 03:26:21]

Looking pretty good. Mostly minor comments / requests for bug filing for later
follow-ups/etc.
Thanks!

https://codereview.chromium.org/1735003004/diff/220001/media/filters/ffmpeg_d...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/1735003004/diff/220001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.cc:1130: std::string track_id =
base::UintToString(stream->id);
nit: the id struct member is an int, not a uint

https://codereview.chromium.org/1735003004/diff/220001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.cc:1138: // Need to fix that and use it as track
id. crbug.com/323183
nit: reactivate that closed bug or file a new one and reference it here (and
below...)

https://codereview.chromium.org/1735003004/diff/220001/media/formats/mp4/box_...
File media/formats/mp4/box_definitions.cc (right):

https://codereview.chromium.org/1735003004/diff/220001/media/formats/mp4/box_...
media/formats/mp4/box_definitions.cc:386: // the Pascal-style string).
A pascal-style string that is shorter than (remaining box bytes + 1) might need
to be truncated to remove potential garbage chars at the end. Should we do any
verification / truncation around suspected pascal-style strings here, too?

https://codereview.chromium.org/1735003004/diff/220001/media/formats/mp4/box_...
media/formats/mp4/box_definitions.cc:673: if (language_code == 0x7fff ||
language_code < 0x400) {
A couple questions about this line:
1) Where do these two magic numbers come from?
2) Is the 16th bit always expected to be zero, or should we mask it off here
before comparing with the magic numbers?

https://codereview.chromium.org/1735003004/diff/220001/media/formats/mp4/box_...
media/formats/mp4/box_definitions.cc:678: lang_chars[2] = 0x60 + (language_code
& 0x1f);
nit: constrain each of lang_chars[0..2] to be within the range of lower case
characters, and if that constraint isn't met, default to "und"?

https://codereview.chromium.org/1735003004/diff/220001/media/formats/mp4/mp4_...
File media/formats/mp4/mp4_stream_parser_unittest.cc (right):

https://codereview.chromium.org/1735003004/diff/220001/media/formats/mp4/mp4_...
media/formats/mp4/mp4_stream_parser_unittest.cc:412:
ParseMP4File("bear-1280x720-av_frag.mp4", 4096);
nit: we probably need (for webm and mp4 across both src= and mp4 unit tests)
more test coverage / media for various language and label, at least. Can you
file a follow-up crbug to do this later please?

https://codereview.chromium.org/1735003004/diff/220001/media/formats/webm/web...
File media/formats/webm/webm_stream_parser_unittest.cc (right):

https://codereview.chromium.org/1735003004/diff/220001/media/formats/webm/web...
media/formats/webm/webm_stream_parser_unittest.cc:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
Aside: thanks for adding this. Until now, most of similar testing for webm was
done in chunkdemuxer unit testing, but this new tracks functionality is
sufficient case to begin independently testing the webm stream parser.

https://codereview.chromium.org/1735003004/diff/220001/media/formats/webm/web...
File media/formats/webm/webm_tracks_parser.cc (right):

https://codereview.chromium.org/1735003004/diff/220001/media/formats/webm/web...
media/formats/webm/webm_tracks_parser.cc:345: track_language_ = str;
Either here or when using track_language_ elsewhere, we should probably sanitize
that it's a real 639-2 string and default to "und" otherwise. IIUC, 639-2 should
be 3 alpha chars, all lower case.

https://codereview.chromium.org/1735003004/diff/220001/media/formats/webm/web...
File media/formats/webm/webm_tracks_parser.h (right):

https://codereview.chromium.org/1735003004/diff/220001/media/formats/webm/web...
media/formats/webm/webm_tracks_parser.h:81: scoped_ptr<MediaTracks>
media_tracks() { return std::move(media_tracks_); }
nit: Using std::move here may have unintended consequence especially if not
documented. What is expected if media_tracks() is called twice in a row, for
instance?

[servolk, 2016-03-07 23:45:02]

https://codereview.chromium.org/1735003004/diff/220001/media/filters/ffmpeg_d...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/1735003004/diff/220001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.cc:1130: std::string track_id =
base::UintToString(stream->id);
On 2016/03/05 03:26:20, wolenetz wrote:
> nit: the id struct member is an int, not a uint

Done.

https://codereview.chromium.org/1735003004/diff/220001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.cc:1138: // Need to fix that and use it as track
id. crbug.com/323183
On 2016/03/05 03:26:20, wolenetz wrote:
> nit: reactivate that closed bug or file a new one and reference it here (and
> below...)

Done.

https://codereview.chromium.org/1735003004/diff/220001/media/formats/mp4/box_...
File media/formats/mp4/box_definitions.cc (right):

https://codereview.chromium.org/1735003004/diff/220001/media/formats/mp4/box_...
media/formats/mp4/box_definitions.cc:386: // the Pascal-style string).
On 2016/03/05 03:26:20, wolenetz wrote:
> A pascal-style string that is shorter than (remaining box bytes + 1) might
need
> to be truncated to remove potential garbage chars at the end. Should we do any
> verification / truncation around suspected pascal-style strings here, too?

Sure, I've added an RCHECK to ensure that the length of pascal-style string is
correct.

https://codereview.chromium.org/1735003004/diff/220001/media/formats/mp4/box_...
media/formats/mp4/box_definitions.cc:673: if (language_code == 0x7fff ||
language_code < 0x400) {
On 2016/03/05 03:26:21, wolenetz wrote:
> A couple questions about this line:
> 1) Where do these two magic numbers come from?
> 2) Is the 16th bit always expected to be zero, or should we mask it off here
> before comparing with the magic numbers?

1) I took those numbers from
https://developer.apple.com/library/mac/documentation/QuickTime/QTFF/QTFFChap...
which is linked from
https://developer.apple.com/library/mac/documentation/QuickTime/QTFF/QTFFChap....
Ideally we'll want to read and use BCP47 language tag from ELNG atom in .mp4
(see https://codereview.chromium.org/1718773002/). But we should probably keep
this as well, as a fallback for better backward compatibility with old streams,
that don't have the ELNG atom.
2) I think it should always be zero, but there's no harm in masking it.

https://codereview.chromium.org/1735003004/diff/220001/media/formats/mp4/box_...
media/formats/mp4/box_definitions.cc:678: lang_chars[2] = 0x60 + (language_code
& 0x1f);
On 2016/03/05 03:26:21, wolenetz wrote:
> nit: constrain each of lang_chars[0..2] to be within the range of lower case
> characters, and if that constraint isn't met, default to "und"?

Done.

https://codereview.chromium.org/1735003004/diff/220001/media/formats/mp4/mp4_...
File media/formats/mp4/mp4_stream_parser_unittest.cc (right):

https://codereview.chromium.org/1735003004/diff/220001/media/formats/mp4/mp4_...
media/formats/mp4/mp4_stream_parser_unittest.cc:412:
ParseMP4File("bear-1280x720-av_frag.mp4", 4096);
On 2016/03/05 03:26:21, wolenetz wrote:
> nit: we probably need (for webm and mp4 across both src= and mp4 unit tests)
> more test coverage / media for various language and label, at least. Can you
> file a follow-up crbug to do this later please?

Opened crbug.com/592717 for tracking this.

https://codereview.chromium.org/1735003004/diff/220001/media/formats/webm/web...
File media/formats/webm/webm_stream_parser_unittest.cc (right):

https://codereview.chromium.org/1735003004/diff/220001/media/formats/webm/web...
media/formats/webm/webm_stream_parser_unittest.cc:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
On 2016/03/05 03:26:21, wolenetz wrote:
> Aside: thanks for adding this. Until now, most of similar testing for webm was
> done in chunkdemuxer unit testing, but this new tracks functionality is
> sufficient case to begin independently testing the webm stream parser.

Acknowledged.

https://codereview.chromium.org/1735003004/diff/220001/media/formats/webm/web...
File media/formats/webm/webm_tracks_parser.cc (right):

https://codereview.chromium.org/1735003004/diff/220001/media/formats/webm/web...
media/formats/webm/webm_tracks_parser.cc:345: track_language_ = str;
On 2016/03/05 03:26:21, wolenetz wrote:
> Either here or when using track_language_ elsewhere, we should probably
sanitize
> that it's a real 639-2 string and default to "und" otherwise. IIUC, 639-2
should
> be 3 alpha chars, all lower case.

Done.

https://codereview.chromium.org/1735003004/diff/220001/media/formats/webm/web...
File media/formats/webm/webm_tracks_parser.h (right):

https://codereview.chromium.org/1735003004/diff/220001/media/formats/webm/web...
media/formats/webm/webm_tracks_parser.h:81: scoped_ptr<MediaTracks>
media_tracks() { return std::move(media_tracks_); }
On 2016/03/05 03:26:21, wolenetz wrote:
> nit: Using std::move here may have unintended consequence especially if not
> documented. What is expected if media_tracks() is called twice in a row, for
> instance?

Well, I agree that it's not super pretty solution, but on the other hand the
fact that this returns scoped_ptr already gives you an indication that you are
being given ownership of this object. So I thought it was ok to do this for now,
to unblock further work.

[servolk, 2016-03-09 19:33:24]

On 2016/03/07 23:45:02, servolk wrote:
>
https://codereview.chromium.org/1735003004/diff/220001/media/filters/ffmpeg_d...
> File media/filters/ffmpeg_demuxer.cc (right):
> 
>
https://codereview.chromium.org/1735003004/diff/220001/media/filters/ffmpeg_d...
> media/filters/ffmpeg_demuxer.cc:1130: std::string track_id =
> base::UintToString(stream->id);
> On 2016/03/05 03:26:20, wolenetz wrote:
> > nit: the id struct member is an int, not a uint
> 
> Done.
> 
>
https://codereview.chromium.org/1735003004/diff/220001/media/filters/ffmpeg_d...
> media/filters/ffmpeg_demuxer.cc:1138: // Need to fix that and use it as track
> id. crbug.com/323183
> On 2016/03/05 03:26:20, wolenetz wrote:
> > nit: reactivate that closed bug or file a new one and reference it here (and
> > below...)
> 
> Done.
> 
>
https://codereview.chromium.org/1735003004/diff/220001/media/formats/mp4/box_...
> File media/formats/mp4/box_definitions.cc (right):
> 
>
https://codereview.chromium.org/1735003004/diff/220001/media/formats/mp4/box_...
> media/formats/mp4/box_definitions.cc:386: // the Pascal-style string).
> On 2016/03/05 03:26:20, wolenetz wrote:
> > A pascal-style string that is shorter than (remaining box bytes + 1) might
> need
> > to be truncated to remove potential garbage chars at the end. Should we do
any
> > verification / truncation around suspected pascal-style strings here, too?
> 
> Sure, I've added an RCHECK to ensure that the length of pascal-style string is
> correct.
> 
>
https://codereview.chromium.org/1735003004/diff/220001/media/formats/mp4/box_...
> media/formats/mp4/box_definitions.cc:673: if (language_code == 0x7fff ||
> language_code < 0x400) {
> On 2016/03/05 03:26:21, wolenetz wrote:
> > A couple questions about this line:
> > 1) Where do these two magic numbers come from?
> > 2) Is the 16th bit always expected to be zero, or should we mask it off here
> > before comparing with the magic numbers?
> 
> 1) I took those numbers from
>
https://developer.apple.com/library/mac/documentation/QuickTime/QTFF/QTFFChap...
> which is linked from
>
https://developer.apple.com/library/mac/documentation/QuickTime/QTFF/QTFFChap....
> Ideally we'll want to read and use BCP47 language tag from ELNG atom in .mp4
> (see https://codereview.chromium.org/1718773002/). But we should probably keep
> this as well, as a fallback for better backward compatibility with old
streams,
> that don't have the ELNG atom.
> 2) I think it should always be zero, but there's no harm in masking it.
> 
>
https://codereview.chromium.org/1735003004/diff/220001/media/formats/mp4/box_...
> media/formats/mp4/box_definitions.cc:678: lang_chars[2] = 0x60 +
(language_code
> & 0x1f);
> On 2016/03/05 03:26:21, wolenetz wrote:
> > nit: constrain each of lang_chars[0..2] to be within the range of lower case
> > characters, and if that constraint isn't met, default to "und"?
> 
> Done.
> 
>
https://codereview.chromium.org/1735003004/diff/220001/media/formats/mp4/mp4_...
> File media/formats/mp4/mp4_stream_parser_unittest.cc (right):
> 
>
https://codereview.chromium.org/1735003004/diff/220001/media/formats/mp4/mp4_...
> media/formats/mp4/mp4_stream_parser_unittest.cc:412:
> ParseMP4File("bear-1280x720-av_frag.mp4", 4096);
> On 2016/03/05 03:26:21, wolenetz wrote:
> > nit: we probably need (for webm and mp4 across both src= and mp4 unit tests)
> > more test coverage / media for various language and label, at least. Can you
> > file a follow-up crbug to do this later please?
> 
> Opened crbug.com/592717 for tracking this.
> 
>
https://codereview.chromium.org/1735003004/diff/220001/media/formats/webm/web...
> File media/formats/webm/webm_stream_parser_unittest.cc (right):
> 
>
https://codereview.chromium.org/1735003004/diff/220001/media/formats/webm/web...
> media/formats/webm/webm_stream_parser_unittest.cc:1: // Copyright 2016 The
> Chromium Authors. All rights reserved.
> On 2016/03/05 03:26:21, wolenetz wrote:
> > Aside: thanks for adding this. Until now, most of similar testing for webm
was
> > done in chunkdemuxer unit testing, but this new tracks functionality is
> > sufficient case to begin independently testing the webm stream parser.
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/1735003004/diff/220001/media/formats/webm/web...
> File media/formats/webm/webm_tracks_parser.cc (right):
> 
>
https://codereview.chromium.org/1735003004/diff/220001/media/formats/webm/web...
> media/formats/webm/webm_tracks_parser.cc:345: track_language_ = str;
> On 2016/03/05 03:26:21, wolenetz wrote:
> > Either here or when using track_language_ elsewhere, we should probably
> sanitize
> > that it's a real 639-2 string and default to "und" otherwise. IIUC, 639-2
> should
> > be 3 alpha chars, all lower case.
> 
> Done.
> 
>
https://codereview.chromium.org/1735003004/diff/220001/media/formats/webm/web...
> File media/formats/webm/webm_tracks_parser.h (right):
> 
>
https://codereview.chromium.org/1735003004/diff/220001/media/formats/webm/web...
> media/formats/webm/webm_tracks_parser.h:81: scoped_ptr<MediaTracks>
> media_tracks() { return std::move(media_tracks_); }
> On 2016/03/05 03:26:21, wolenetz wrote:
> > nit: Using std::move here may have unintended consequence especially if not
> > documented. What is expected if media_tracks() is called twice in a row, for
> > instance?
> 
> Well, I agree that it's not super pretty solution, but on the other hand the
> fact that this returns scoped_ptr already gives you an indication that you are
> being given ownership of this object. So I thought it was ok to do this for
now,
> to unblock further work.

ping

[philipj_slow, 2016-03-10 13:57:27]

Continuing the discussion of BCP 47 from
https://codereview.chromium.org/1735803002/#msg11

The spec that requires BCP 47 for these fields is HTML, close to
https://html.spec.whatwg.org/multipage/embedded-content.html#audiotracklist-a...

"The AudioTrack.language and VideoTrack.language attributes must return the BCP
47 language tag of the language of the track, if it has one, or the empty string
otherwise. If the user agent is not able to express that language as a BCP 47
language tag (for example because the language information in the media
resource's format is a free-form string without a defined interpretation), then
the method must return the empty string, as if the track had no language."

If https://dev.w3.org/html5/html-sourcing-inband-tracks/ says things that fails
to ensure BCP 47, then it'll have to be fixed, or maybe the BCP 47 requirement
should be revisited. I don't think that would really solve the problem, though,
because no matter the requirement is there will be *some* media format that
requires some fixups to comply.

For MPEG-4 or any other format which uses 3-character 639-2, I think the only
solution would be to have a (generated) map from 3-character strings to
2-character BCP 47-valid string, and fall back to the empty string.

[philipj_slow, 2016-03-10 14:13:54]

philipj@opera.com changed reviewers:
+ philipj@opera.com

[philipj_slow, 2016-03-10 14:13:54]

Oh right, there is also the issue that it appears that "eng" is used for at
least some MPEG-4 files that aren't actually in English, perhaps because it's
the default in some popular muxer. Can you have a look at some MPEG-4 files in
the wild to see if most of them are "eng"? If so I'm not sure what solution I'd
propose, but it merits some thought. Checking if WebM has a similar problem
would also be nice.

[wolenetz, 2016-03-10 19:38:35]

On 2016/03/10 14:13:54, philipj_UTC7 wrote:
> Oh right, there is also the issue that it appears that "eng" is used for at
> least some MPEG-4 files that aren't actually in English, perhaps because it's
> the default in some popular muxer. Can you have a look at some MPEG-4 files in
> the wild to see if most of them are "eng"? If so I'm not sure what solution
I'd
> propose, but it merits some thought. Checking if WebM has a similar problem
> would also be nice.

servolk/philipj@ this sounds like a potential spec bug on
html-sourcing-inband-tracks, or alternatively on HTML5 (and as extended by MSE)
track attribute specs. I can help with MSE, but that depends on HTML5 and on
html-source-inband-tracks, so really I would prefer spec bugs be filed to
understand better which of BCP 47 or ISO639-2 is more appropriate.

As it stands right now, it looks like Chromium has support for mapping ISO639-2
codes into BCP 47 codes (and knowing when there isn't a corresponding BCP 47
code, too): see LANGUAGE and LANGUAGE3 in:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/icu/so...

In the interests of making progress and not letting BCP 47 vs ISO639-2 language
sourcing prevent progress elsewhere, please:
1) file a crbug and reference in TODO in this CL to follow-up on the spec
questions,
2) do what the specs currently say (source them from mp4 as ISO639-2, expose
them to app as BCP 47, which IIUC means we need to use something like that
LANGUAGE and LANGUAGE3 stuff at some point on the Chromium side while populating
the language attribute of a MediaTrack. I'm fine also for this second piece to
be left as a TODO with yet another crbug, so this CL doesn't stall on getting
that translation into BCP-47 figured out (and it might even be throwaway work if
indeed ISO639-2 is better than BCP-47, but the HTML5 specs are too restrictive
now and get changed to allow ISO639-2).

[wolenetz, 2016-03-10 19:53:29]

Almost there :)
I can do faster turnaround on reviews today, too.

https://codereview.chromium.org/1735003004/diff/220001/media/formats/mp4/box_...
File media/formats/mp4/box_definitions.cc (right):

https://codereview.chromium.org/1735003004/diff/220001/media/formats/mp4/box_...
media/formats/mp4/box_definitions.cc:673: if (language_code == 0x7fff ||
language_code < 0x400) {
On 2016/03/07 23:45:01, servolk wrote:
> On 2016/03/05 03:26:21, wolenetz wrote:
> > A couple questions about this line:
> > 1) Where do these two magic numbers come from?
> > 2) Is the 16th bit always expected to be zero, or should we mask it off here
> > before comparing with the magic numbers?
> 
> 1) I took those numbers from
>
https://developer.apple.com/library/mac/documentation/QuickTime/QTFF/QTFFChap...
> which is linked from
>
https://developer.apple.com/library/mac/documentation/QuickTime/QTFF/QTFFChap....
> Ideally we'll want to read and use BCP47 language tag from ELNG atom in .mp4
> (see https://codereview.chromium.org/1718773002/). But we should probably keep
> this as well, as a fallback for better backward compatibility with old
streams,
> that don't have the ELNG atom.
> 2) I think it should always be zero, but there's no harm in masking it.

Acknowledged.

https://codereview.chromium.org/1735003004/diff/220001/media/formats/mp4/mp4_...
File media/formats/mp4/mp4_stream_parser_unittest.cc (right):

https://codereview.chromium.org/1735003004/diff/220001/media/formats/mp4/mp4_...
media/formats/mp4/mp4_stream_parser_unittest.cc:412:
ParseMP4File("bear-1280x720-av_frag.mp4", 4096);
On 2016/03/07 23:45:01, servolk wrote:
> On 2016/03/05 03:26:21, wolenetz wrote:
> > nit: we probably need (for webm and mp4 across both src= and mp4 unit tests)
> > more test coverage / media for various language and label, at least. Can you
> > file a follow-up crbug to do this later please?
> 
> Opened crbug.com/592717 for tracking this.

Acknowledged.

https://codereview.chromium.org/1735003004/diff/220001/media/formats/webm/web...
File media/formats/webm/webm_tracks_parser.h (right):

https://codereview.chromium.org/1735003004/diff/220001/media/formats/webm/web...
media/formats/webm/webm_tracks_parser.h:81: scoped_ptr<MediaTracks>
media_tracks() { return std::move(media_tracks_); }
On 2016/03/07 23:45:02, servolk wrote:
> On 2016/03/05 03:26:21, wolenetz wrote:
> > nit: Using std::move here may have unintended consequence especially if not
> > documented. What is expected if media_tracks() is called twice in a row, for
> > instance?
> 
> Well, I agree that it's not super pretty solution, but on the other hand the
> fact that this returns scoped_ptr already gives you an indication that you are
> being given ownership of this object. So I thought it was ok to do this for
now,
> to unblock further work.

I understand. Please then document in the .h for this method that at most one
call to it is supported. Even better, enforce that with some boolean + a DCHECK
gating that at most one call can happen.
Alternatively, is this a place where scoped_refptr might be better, so that this
parser isn't so tightly bound to its caller only calling this method once?

https://codereview.chromium.org/1735003004/diff/240001/media/formats/mp4/box_...
File media/formats/mp4/box_definitions.cc (right):

https://codereview.chromium.org/1735003004/diff/240001/media/formats/mp4/box_...
media/formats/mp4/box_definitions.cc:393: RCHECK(name_bytes[0] ==
(name_bytes.size() - 1));
Is it possible that a 0-byte name_bytes might occur legally and let us reach
l.389 in this patch set? If so, please condition this whole block (l.389-l.395)
on actually having some name_bytes; otherwise we risk out-of-range access.

https://codereview.chromium.org/1735003004/diff/240001/media/formats/mp4/box_...
media/formats/mp4/box_definitions.cc:658: bool MediaHeader::Parse(BoxReader*
reader) {
aside: please reference potential for mp4 ELNG to have better/different info in
the requested-to-be-filed crbug around figuring out if the specs are being sane
w.r.t. sourcing and exposing track language attribute.

[servolk, 2016-03-10 22:05:49]

On 2016/03/10 19:38:35, wolenetz wrote:
> On 2016/03/10 14:13:54, philipj_UTC7 wrote:
> > Oh right, there is also the issue that it appears that "eng" is used for at
> > least some MPEG-4 files that aren't actually in English, perhaps because
it's
> > the default in some popular muxer. Can you have a look at some MPEG-4 files
in
> > the wild to see if most of them are "eng"? If so I'm not sure what solution
> I'd
> > propose, but it merits some thought. Checking if WebM has a similar problem
> > would also be nice.
> 
> servolk/philipj@ this sounds like a potential spec bug on
> html-sourcing-inband-tracks, or alternatively on HTML5 (and as extended by
MSE)
> track attribute specs. I can help with MSE, but that depends on HTML5 and on
> html-source-inband-tracks, so really I would prefer spec bugs be filed to
> understand better which of BCP 47 or ISO639-2 is more appropriate.
> 
> As it stands right now, it looks like Chromium has support for mapping
ISO639-2
> codes into BCP 47 codes (and knowing when there isn't a corresponding BCP 47
> code, too): see LANGUAGE and LANGUAGE3 in:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/icu/so...
> 
> In the interests of making progress and not letting BCP 47 vs ISO639-2
language
> sourcing prevent progress elsewhere, please:
> 1) file a crbug and reference in TODO in this CL to follow-up on the spec
> questions,
> 2) do what the specs currently say (source them from mp4 as ISO639-2, expose
> them to app as BCP 47, which IIUC means we need to use something like that
> LANGUAGE and LANGUAGE3 stuff at some point on the Chromium side while
populating
> the language attribute of a MediaTrack. I'm fine also for this second piece to
> be left as a TODO with yet another crbug, so this CL doesn't stall on getting
> that translation into BCP-47 figured out (and it might even be throwaway work
if
> indeed ISO639-2 is better than BCP-47, but the HTML5 specs are too restrictive
> now and get changed to allow ISO639-2).

Done: I've opened bug crbug.com/593904 to track this. I've looked at the
available apis in third_party/icu/source/common/uloc.cpp and couldn't quickly
find a way to convert ISO639-2 lang code into BCP-47 string. IIUC BCP-47 is a
better and more recent standard for specifying languages/locales, so probably
it's the INBAND media track sourcing track that needs to be updated to use
BCP-47. But where do I file a bug for that spec?

[servolk, 2016-03-10 22:25:38]

https://codereview.chromium.org/1735003004/diff/220001/media/formats/webm/web...
File media/formats/webm/webm_tracks_parser.h (right):

https://codereview.chromium.org/1735003004/diff/220001/media/formats/webm/web...
media/formats/webm/webm_tracks_parser.h:81: scoped_ptr<MediaTracks>
media_tracks() { return std::move(media_tracks_); }
On 2016/03/10 19:53:29, wolenetz wrote:
> On 2016/03/07 23:45:02, servolk wrote:
> > On 2016/03/05 03:26:21, wolenetz wrote:
> > > nit: Using std::move here may have unintended consequence especially if
not
> > > documented. What is expected if media_tracks() is called twice in a row,
for
> > > instance?
> > 
> > Well, I agree that it's not super pretty solution, but on the other hand the
> > fact that this returns scoped_ptr already gives you an indication that you
are
> > being given ownership of this object. So I thought it was ok to do this for
> now,
> > to unblock further work.
> 
> I understand. Please then document in the .h for this method that at most one
> call to it is supported. Even better, enforce that with some boolean + a
DCHECK
> gating that at most one call can happen.
> Alternatively, is this a place where scoped_refptr might be better, so that
this
> parser isn't so tightly bound to its caller only calling this method once?

Done.
Note that this method is in the WebMTracksParser class, which is probably only
going to be ever used directly in WebMStreamParser + unit tests (since this
class is too low-level to be useful on it's own, unlike WebMStreamParser). So it
doesn't seem like a big deal to me. We could probably try to use scoped_refptr,
but then we'd have to make MediaTracks refcounted and update all other places
that use it. I don't see a lot of benefit in doing that atm. But of course, we
can always reconsider in the future.

https://codereview.chromium.org/1735003004/diff/240001/media/formats/mp4/box_...
File media/formats/mp4/box_definitions.cc (right):

https://codereview.chromium.org/1735003004/diff/240001/media/formats/mp4/box_...
media/formats/mp4/box_definitions.cc:393: RCHECK(name_bytes[0] ==
(name_bytes.size() - 1));
On 2016/03/10 19:53:29, wolenetz wrote:
> Is it possible that a 0-byte name_bytes might occur legally and let us reach
> l.389 in this patch set? If so, please condition this whole block
(l.389-l.395)
> on actually having some name_bytes; otherwise we risk out-of-range access.

Done.

https://codereview.chromium.org/1735003004/diff/240001/media/formats/mp4/box_...
media/formats/mp4/box_definitions.cc:658: bool MediaHeader::Parse(BoxReader*
reader) {
On 2016/03/10 19:53:29, wolenetz wrote:
> aside: please reference potential for mp4 ELNG to have better/different info
in
> the requested-to-be-filed crbug around figuring out if the specs are being
sane
> w.r.t. sourcing and exposing track language attribute.

Done: crbug.com/593904

[wolenetz, 2016-03-11 00:14:03]

On 2016/03/10 22:05:49, servolk wrote:
> On 2016/03/10 19:38:35, wolenetz wrote:
> > On 2016/03/10 14:13:54, philipj_UTC7 wrote:
> > > Oh right, there is also the issue that it appears that "eng" is used for
at
> > > least some MPEG-4 files that aren't actually in English, perhaps because
> it's
> > > the default in some popular muxer. Can you have a look at some MPEG-4
files
> in
> > > the wild to see if most of them are "eng"? If so I'm not sure what
solution
> > I'd
> > > propose, but it merits some thought. Checking if WebM has a similar
problem
> > > would also be nice.
> > 
> > servolk/philipj@ this sounds like a potential spec bug on
> > html-sourcing-inband-tracks, or alternatively on HTML5 (and as extended by
> MSE)
> > track attribute specs. I can help with MSE, but that depends on HTML5 and on
> > html-source-inband-tracks, so really I would prefer spec bugs be filed to
> > understand better which of BCP 47 or ISO639-2 is more appropriate.
> > 
> > As it stands right now, it looks like Chromium has support for mapping
> ISO639-2
> > codes into BCP 47 codes (and knowing when there isn't a corresponding BCP 47
> > code, too): see LANGUAGE and LANGUAGE3 in:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/icu/so...
> > 
> > In the interests of making progress and not letting BCP 47 vs ISO639-2
> language
> > sourcing prevent progress elsewhere, please:
> > 1) file a crbug and reference in TODO in this CL to follow-up on the spec
> > questions,
> > 2) do what the specs currently say (source them from mp4 as ISO639-2, expose
> > them to app as BCP 47, which IIUC means we need to use something like that
> > LANGUAGE and LANGUAGE3 stuff at some point on the Chromium side while
> populating
> > the language attribute of a MediaTrack. I'm fine also for this second piece
to
> > be left as a TODO with yet another crbug, so this CL doesn't stall on
getting
> > that translation into BCP-47 figured out (and it might even be throwaway
work
> if
> > indeed ISO639-2 is better than BCP-47, but the HTML5 specs are too
restrictive
> > now and get changed to allow ISO639-2).
> 
> Done: I've opened bug crbug.com/593904 to track this. I've looked at the
> available apis in third_party/icu/source/common/uloc.cpp and couldn't quickly
> find a way to convert ISO639-2 lang code into BCP-47 string. IIUC BCP-47 is a
> better and more recent standard for specifying languages/locales, so probably
> it's the INBAND media track sourcing track that needs to be updated to use
> BCP-47. But where do I file a bug for that spec?

That inband track attribute sourcing spec is a draft. It asks for feedback to be
sent to public-inbandtracks at w3.org.
If needed, we could contact it's editors directly, but I recommend first trying
to the w3.org list.

[wolenetz, 2016-03-11 00:17:43]

lgtm

[servolk, 2016-03-11 00:18:31]

On 2016/03/11 00:17:43, wolenetz wrote:
> lgtm

thanks!

[servolk, 2016-03-11 00:18:37]

The CQ bit was checked by servolk@chromium.org

[commit-bot: I haz the power, 2016-03-11 00:19:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1735003004/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1735003004/260001

[commit-bot: I haz the power, 2016-03-11 00:58:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-11 00:59:00]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[servolk, 2016-03-11 01:02:47]

The CQ bit was checked by servolk@chromium.org

[commit-bot: I haz the power, 2016-03-11 01:03:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1735003004/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1735003004/260001

[commit-bot: I haz the power, 2016-03-11 01:46:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-11 01:46:25]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[servolk, 2016-03-11 01:49:34]

The CQ bit was checked by servolk@chromium.org

[servolk, 2016-03-11 01:49:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
wolenetz@chromium.org
Link to the patchset: https://codereview.chromium.org/1735003004/#ps280001
(title: "rebase")

[commit-bot: I haz the power, 2016-03-11 01:50:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1735003004/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1735003004/280001

[commit-bot: I haz the power, 2016-03-11 02:21:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-11 02:21:07]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[servolk, 2016-03-11 02:24:52]

The CQ bit was checked by servolk@chromium.org

[commit-bot: I haz the power, 2016-03-11 02:25:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1735003004/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1735003004/280001

[commit-bot: I haz the power, 2016-03-11 02:58:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-11 02:58:14]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[servolk, 2016-03-11 03:08:31]

The CQ bit was checked by servolk@chromium.org

[commit-bot: I haz the power, 2016-03-11 03:08:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1735003004/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1735003004/280001

[commit-bot: I haz the power, 2016-03-11 05:33:14]

Description was changed from

==========
Implement reading of media track info from WebM and MP4 containers

BUG=590085
==========

to

==========
Implement reading of media track info from WebM and MP4 containers

BUG=590085
==========

[commit-bot: I haz the power, 2016-03-11 05:33:15]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2016-03-11 05:34:36]

Description was changed from

==========
Implement reading of media track info from WebM and MP4 containers

BUG=590085
==========

to

==========
Implement reading of media track info from WebM and MP4 containers

BUG=590085

Committed: https://crrev.com/d307b5f1d4fe75cb8a3d1e080d4890ae757d1634
Cr-Commit-Position: refs/heads/master@{#380532}
==========

[commit-bot: I haz the power, 2016-03-11 05:34:38]

Patchset 15 (id:??) landed as
https://crrev.com/d307b5f1d4fe75cb8a3d1e080d4890ae757d1634
Cr-Commit-Position: refs/heads/master@{#380532}