Media Galleries API Metadata: Add audio video metadata extractor.

Media Galleries API Metadata: Add audio video metadata extractor.

This CL adds a class for extraction of audio and video metadata using ffmpeg.

This is needed to implement the Media Galleries Metadata API for Chrome extensions.

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

[tommycli, 2014-01-02 17:23:34]

acolwell: May I have a review? Thx.

[tommycli, 2014-01-02 17:23:58]

+thestig

[acolwell GONE FROM CHROMIUM, 2014-01-03 19:14:04]

https://codereview.chromium.org/121383002/diff/290001/chrome/common/extension...
File chrome/common/extensions/api/media_galleries.idl (right):

https://codereview.chromium.org/121383002/diff/290001/chrome/common/extension...
chrome/common/extensions/api/media_galleries.idl:78: long? durationSeconds;
nit: Consider using a double since time in HTMLMediaElements are represented as
doubles. Also s/durationSeconds/duration/ to match the HTMLMediaElement field
that would have a equivalent value if you actually loaded the file.

https://codereview.chromium.org/121383002/diff/290001/chrome/common/extension...
chrome/common/extensions/api/media_galleries.idl:80: // FFmpeg-defined generic
fields.
I don't think it is wise to expose these fields in terms of what FFmpeg returns.
These seem more like ID3 type fields so you should probably reference
appropriate parts of that spec for each one so these values are well defined.

Another reason to not bind this to FFmpeg is that FFmpeg isn't used on Android.

https://codereview.chromium.org/121383002/diff/290001/chrome/utility/media_ga...
File chrome/utility/media_galleries/media_metadata_parser.cc (right):

https://codereview.chromium.org/121383002/diff/290001/chrome/utility/media_ga...
chrome/utility/media_galleries/media_metadata_parser.cc:62:
SetStringScopedPtrFromDict(tags, "album", &metadata->album);
This seems brittle and unnecessarily dependent on FFmpeg's choice of tag names.
I think the extractor should have explicit methods for each of these pieces of
data like you have for the width and height. That way only the extractor has to
care about FFmpeg details.

https://codereview.chromium.org/121383002/diff/290001/chrome/utility/media_ga...
chrome/utility/media_galleries/media_metadata_parser.cc:90:
PopulateAudioVideoMetadata(source_.get(), metadata_.get());
nit: Why don't you simply make this a private member function so you don't have
to pass in these members?

https://codereview.chromium.org/121383002/diff/290001/chrome/utility/media_ga...
File chrome/utility/media_galleries/media_metadata_parser.h (right):

https://codereview.chromium.org/121383002/diff/290001/chrome/utility/media_ga...
chrome/utility/media_galleries/media_metadata_parser.h:31:
MediaMetadataParser(media::DataSource* source, const std::string& mime_type);
nit: Make this a scoped_ptr so the ownership transfer is obvious at the call
site.

https://codereview.chromium.org/121383002/diff/290001/media/base/audio_video_...
File media/base/audio_video_metadata_extractor.cc (right):

https://codereview.chromium.org/121383002/diff/290001/media/base/audio_video_...
media/base/audio_video_metadata_extractor.cc:91: // metadata to contained
streams instead the container itself, like OGG.
This seems brittle. How about just taking the first value for each property you
are looking for? At least that would allow you to catch metadata that is
sprinkled across multiple tracks.

https://codereview.chromium.org/121383002/diff/290001/media/base/audio_video_...
File media/base/audio_video_metadata_extractor.h (right):

https://codereview.chromium.org/121383002/diff/290001/media/base/audio_video_...
media/base/audio_video_metadata_extractor.h:29: const std::string& GetFormat();
nit: I think this should be removed. It unnecessarily leaks FFmpeg info and is
only used for testing.

https://codereview.chromium.org/121383002/diff/290001/media/base/audio_video_...
media/base/audio_video_metadata_extractor.h:30: int GetDurationSeconds();
Use double or TimeDelta for this like the other media code does.

https://codereview.chromium.org/121383002/diff/290001/media/base/audio_video_...
media/base/audio_video_metadata_extractor.h:33: int GetWidth();
nit: s/GetWidth/width/ to match Chromium style for simple accessors.

[Lei Zhang, 2014-01-03 23:00:14]

https://codereview.chromium.org/121383002/diff/290001/chrome/common/extension...
File chrome/common/extensions/api/media_galleries.idl (right):

https://codereview.chromium.org/121383002/diff/290001/chrome/common/extension...
chrome/common/extensions/api/media_galleries.idl:80: // FFmpeg-defined generic
fields.
On 2014/01/03 19:14:04, acolwell wrote:
> Another reason to not bind this to FFmpeg is that FFmpeg isn't used on
Android.

Chrome extensions and apps are not available on mobile platforms either, FWIW.

[tommycli, 2014-01-03 23:08:36]

acolwell: thx for review. I tried to address your comments.

https://codereview.chromium.org/121383002/diff/290001/chrome/common/extension...
File chrome/common/extensions/api/media_galleries.idl (right):

https://codereview.chromium.org/121383002/diff/290001/chrome/common/extension...
chrome/common/extensions/api/media_galleries.idl:78: long? durationSeconds;
On 2014/01/03 19:14:04, acolwell wrote:
> nit: Consider using a double since time in HTMLMediaElements are represented
as
> doubles. Also s/durationSeconds/duration/ to match the HTMLMediaElement field
> that would have a equivalent value if you actually loaded the file.

Done.

https://codereview.chromium.org/121383002/diff/290001/chrome/common/extension...
chrome/common/extensions/api/media_galleries.idl:80: // FFmpeg-defined generic
fields.
Comment might be misleading. FFmpeg inspired the tag names, but we could
implement this with an alternate backend also.

On 2014/01/03 19:14:04, acolwell wrote:
> I don't think it is wise to expose these fields in terms of what FFmpeg
returns.
> These seem more like ID3 type fields so you should probably reference
> appropriate parts of that spec for each one so these values are well defined.
> 
> Another reason to not bind this to FFmpeg is that FFmpeg isn't used on
Android.

https://codereview.chromium.org/121383002/diff/290001/chrome/utility/media_ga...
File chrome/utility/media_galleries/media_metadata_parser.cc (right):

https://codereview.chromium.org/121383002/diff/290001/chrome/utility/media_ga...
chrome/utility/media_galleries/media_metadata_parser.cc:62:
SetStringScopedPtrFromDict(tags, "album", &metadata->album);
On 2014/01/03 19:14:04, acolwell wrote:
> This seems brittle and unnecessarily dependent on FFmpeg's choice of tag
names.
> I think the extractor should have explicit methods for each of these pieces of
> data like you have for the width and height. That way only the extractor has
to
> care about FFmpeg details.

Done.

https://codereview.chromium.org/121383002/diff/290001/chrome/utility/media_ga...
chrome/utility/media_galleries/media_metadata_parser.cc:90:
PopulateAudioVideoMetadata(source_.get(), metadata_.get());
On 2014/01/03 19:14:04, acolwell wrote:
> nit: Why don't you simply make this a private member function so you don't
have
> to pass in these members?

Done.

https://codereview.chromium.org/121383002/diff/290001/chrome/utility/media_ga...
File chrome/utility/media_galleries/media_metadata_parser.h (right):

https://codereview.chromium.org/121383002/diff/290001/chrome/utility/media_ga...
chrome/utility/media_galleries/media_metadata_parser.h:31:
MediaMetadataParser(media::DataSource* source, const std::string& mime_type);
On 2014/01/03 19:14:04, acolwell wrote:
> nit: Make this a scoped_ptr so the ownership transfer is obvious at the call
> site.

Done.

https://codereview.chromium.org/121383002/diff/290001/media/base/audio_video_...
File media/base/audio_video_metadata_extractor.cc (right):

https://codereview.chromium.org/121383002/diff/290001/media/base/audio_video_...
media/base/audio_video_metadata_extractor.cc:91: // metadata to contained
streams instead the container itself, like OGG.
On 2014/01/03 19:14:04, acolwell wrote:
> This seems brittle. How about just taking the first value for each property
you
> are looking for? At least that would allow you to catch metadata that is
> sprinkled across multiple tracks.

Done.

https://codereview.chromium.org/121383002/diff/290001/media/base/audio_video_...
File media/base/audio_video_metadata_extractor.h (right):

https://codereview.chromium.org/121383002/diff/290001/media/base/audio_video_...
media/base/audio_video_metadata_extractor.h:29: const std::string& GetFormat();
On 2014/01/03 19:14:04, acolwell wrote:
> nit: I think this should be removed. It unnecessarily leaks FFmpeg info and is
> only used for testing.

Done.

https://codereview.chromium.org/121383002/diff/290001/media/base/audio_video_...
media/base/audio_video_metadata_extractor.h:30: int GetDurationSeconds();
On 2014/01/03 19:14:04, acolwell wrote:
> Use double or TimeDelta for this like the other media code does.

Done.

https://codereview.chromium.org/121383002/diff/290001/media/base/audio_video_...
media/base/audio_video_metadata_extractor.h:33: int GetWidth();
On 2014/01/03 19:14:04, acolwell wrote:
> nit: s/GetWidth/width/ to match Chromium style for simple accessors.

Done.

[tommycli, 2014-01-06 23:45:49]

acolwell: ping

[acolwell GONE FROM CHROMIUM, 2014-01-07 00:27:23]

lgtm % nits

https://codereview.chromium.org/121383002/diff/620001/media/base/audio_video_...
File media/base/audio_video_metadata_extractor.cc (right):

https://codereview.chromium.org/121383002/diff/620001/media/base/audio_video_...
media/base/audio_video_metadata_extractor.cc:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
ditto

https://codereview.chromium.org/121383002/diff/620001/media/base/audio_video_...
media/base/audio_video_metadata_extractor.cc:44: base::StringToInt(tag->value,
destination);
nit: You might want to stage the parsed value in a temporary and verify it's >=
0 so that unwanted negative values can't sneak in here.

https://codereview.chromium.org/121383002/diff/620001/media/base/audio_video_...
File media/base/audio_video_metadata_extractor.h (right):

https://codereview.chromium.org/121383002/diff/620001/media/base/audio_video_...
media/base/audio_video_metadata_extractor.h:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
nit: Should be 2014 now.

https://codereview.chromium.org/121383002/diff/620001/media/base/audio_video_...
media/base/audio_video_metadata_extractor.h:5: #ifndef
MEDIA_BASE_MEDIA_FFMPEG_METADATA_EXTRACTOR_H_
nit: Change guard to MEDIA_BASE_AUDIO_VIDEO_METADATA_EXTRACTOR_H_ here and below

https://codereview.chromium.org/121383002/diff/620001/media/base/audio_video_...
media/base/audio_video_metadata_extractor.h:55: std::map<std::string,
std::string> tags_;
nit: remove since it isn't used anymore.

https://codereview.chromium.org/121383002/diff/620001/media/base/audio_video_...
File media/base/audio_video_metadata_extractor_unittest.cc (right):

https://codereview.chromium.org/121383002/diff/620001/media/base/audio_video_...
media/base/audio_video_metadata_extractor_unittest.cc:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
ditto

[tommycli, 2014-01-07 00:55:28]

acolwell: thanks!
thestig: Need OWNER review for chrome/common && chrome/utility. thanks

https://codereview.chromium.org/121383002/diff/620001/media/base/audio_video_...
File media/base/audio_video_metadata_extractor.cc (right):

https://codereview.chromium.org/121383002/diff/620001/media/base/audio_video_...
media/base/audio_video_metadata_extractor.cc:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
On 2014/01/07 00:27:24, acolwell wrote:
> ditto

Done.

https://codereview.chromium.org/121383002/diff/620001/media/base/audio_video_...
media/base/audio_video_metadata_extractor.cc:44: base::StringToInt(tag->value,
destination);
On 2014/01/07 00:27:24, acolwell wrote:
> nit: You might want to stage the parsed value in a temporary and verify it's
>=
> 0 so that unwanted negative values can't sneak in here.

Done.

https://codereview.chromium.org/121383002/diff/620001/media/base/audio_video_...
File media/base/audio_video_metadata_extractor.h (right):

https://codereview.chromium.org/121383002/diff/620001/media/base/audio_video_...
media/base/audio_video_metadata_extractor.h:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
On 2014/01/07 00:27:24, acolwell wrote:
> nit: Should be 2014 now. 

Done.

https://codereview.chromium.org/121383002/diff/620001/media/base/audio_video_...
media/base/audio_video_metadata_extractor.h:5: #ifndef
MEDIA_BASE_MEDIA_FFMPEG_METADATA_EXTRACTOR_H_
On 2014/01/07 00:27:24, acolwell wrote:
> nit: Change guard to MEDIA_BASE_AUDIO_VIDEO_METADATA_EXTRACTOR_H_ here and
below

Done.

https://codereview.chromium.org/121383002/diff/620001/media/base/audio_video_...
media/base/audio_video_metadata_extractor.h:55: std::map<std::string,
std::string> tags_;
On 2014/01/07 00:27:24, acolwell wrote:
> nit: remove since it isn't used anymore.

Done.

https://codereview.chromium.org/121383002/diff/620001/media/base/audio_video_...
File media/base/audio_video_metadata_extractor_unittest.cc (right):

https://codereview.chromium.org/121383002/diff/620001/media/base/audio_video_...
media/base/audio_video_metadata_extractor_unittest.cc:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
On 2014/01/07 00:27:24, acolwell wrote:
> ditto

Done.

[Lei Zhang, 2014-01-07 21:44:25]

https://codereview.chromium.org/121383002/diff/670001/chrome/utility/media_ga...
File chrome/utility/media_galleries/media_metadata_parser.cc (right):

https://codereview.chromium.org/121383002/diff/670001/chrome/utility/media_ga...
chrome/utility/media_galleries/media_metadata_parser.cc:7: #include <map>
not used

https://codereview.chromium.org/121383002/diff/670001/media/base/audio_video_...
File media/base/audio_video_metadata_extractor.cc (right):

https://codereview.chromium.org/121383002/diff/670001/media/base/audio_video_...
media/base/audio_video_metadata_extractor.cc:7: #include <map>
nit: remove

https://codereview.chromium.org/121383002/diff/670001/media/base/audio_video_...
media/base/audio_video_metadata_extractor.cc:21: static void OnError(bool*
called) {
nit: no need for static

https://codereview.chromium.org/121383002/diff/670001/media/base/audio_video_...
media/base/audio_video_metadata_extractor.cc:21: static void OnError(bool*
called) {
Isn't |succeeded| more clear than |called| ?

https://codereview.chromium.org/121383002/diff/670001/media/base/audio_video_...
media/base/audio_video_metadata_extractor.cc:45: *destination < 0) {
it's cheaper to check *dest first before doing the StringToInt conversion.

https://codereview.chromium.org/121383002/diff/670001/media/base/audio_video_...
media/base/audio_video_metadata_extractor.cc:91: for (unsigned int i = 0; i <
format_context->nb_streams; ++i) {
Isn't |format_context->nb_streams| an int, and there by |i| should be as well?

https://codereview.chromium.org/121383002/diff/670001/media/base/audio_video_...
media/base/audio_video_metadata_extractor.cc:201: StringToLowerASCII(&key);
Probably better to do this in ExtractString(), rather than require the passed in
key to be lowercase. You can also use LowerCaseEqualsASCII() to avoid the
conversion.

https://codereview.chromium.org/121383002/diff/670001/media/base/audio_video_...
File media/base/audio_video_metadata_extractor.h (right):

https://codereview.chromium.org/121383002/diff/670001/media/base/audio_video_...
media/base/audio_video_metadata_extractor.h:8: #include <map>
nit: remove

https://codereview.chromium.org/121383002/diff/670001/media/base/audio_video_...
media/base/audio_video_metadata_extractor.h:14: struct AVDictionary;
Are you going to use AVDictionary outside of this class? If not, then make it
private?

https://codereview.chromium.org/121383002/diff/670001/media/base/audio_video_...
media/base/audio_video_metadata_extractor.h:29: bool Extract(media::DataSource*
source);
nit:: you can omit media:: inside namespace media.

https://codereview.chromium.org/121383002/diff/670001/media/base/audio_video_...
media/base/audio_video_metadata_extractor.h:31: double duration() const;
Mention this is in seconds.

[tommycli, 2014-01-07 23:12:35]

thestig: thx for review. tried to address your issues.

https://codereview.chromium.org/121383002/diff/670001/chrome/utility/media_ga...
File chrome/utility/media_galleries/media_metadata_parser.cc (right):

https://codereview.chromium.org/121383002/diff/670001/chrome/utility/media_ga...
chrome/utility/media_galleries/media_metadata_parser.cc:7: #include <map>
On 2014/01/07 21:44:25, Lei Zhang wrote:
> not used

Done.

https://codereview.chromium.org/121383002/diff/670001/media/base/audio_video_...
File media/base/audio_video_metadata_extractor.cc (right):

https://codereview.chromium.org/121383002/diff/670001/media/base/audio_video_...
media/base/audio_video_metadata_extractor.cc:7: #include <map>
On 2014/01/07 21:44:25, Lei Zhang wrote:
> nit: remove

Done.

https://codereview.chromium.org/121383002/diff/670001/media/base/audio_video_...
media/base/audio_video_metadata_extractor.cc:21: static void OnError(bool*
called) {
On 2014/01/07 21:44:25, Lei Zhang wrote:
> Isn't |succeeded| more clear than |called| ?

Done.

https://codereview.chromium.org/121383002/diff/670001/media/base/audio_video_...
media/base/audio_video_metadata_extractor.cc:21: static void OnError(bool*
called) {
On 2014/01/07 21:44:25, Lei Zhang wrote:
> nit: no need for static

Done.

https://codereview.chromium.org/121383002/diff/670001/media/base/audio_video_...
media/base/audio_video_metadata_extractor.cc:45: *destination < 0) {
On 2014/01/07 21:44:25, Lei Zhang wrote:
> it's cheaper to check *dest first before doing the StringToInt conversion.

Done.

https://codereview.chromium.org/121383002/diff/670001/media/base/audio_video_...
media/base/audio_video_metadata_extractor.cc:91: for (unsigned int i = 0; i <
format_context->nb_streams; ++i) {
On 2014/01/07 21:44:25, Lei Zhang wrote:
> Isn't |format_context->nb_streams| an int, and there by |i| should be as well?

I believe nb_streams is an unsigned int.

https://codereview.chromium.org/121383002/diff/670001/media/base/audio_video_...
media/base/audio_video_metadata_extractor.cc:201: StringToLowerASCII(&key);
On 2014/01/07 21:44:25, Lei Zhang wrote:
> Probably better to do this in ExtractString(), rather than require the passed
in
> key to be lowercase. You can also use LowerCaseEqualsASCII() to avoid the
> conversion.

Done.

https://codereview.chromium.org/121383002/diff/670001/media/base/audio_video_...
File media/base/audio_video_metadata_extractor.h (right):

https://codereview.chromium.org/121383002/diff/670001/media/base/audio_video_...
media/base/audio_video_metadata_extractor.h:8: #include <map>
On 2014/01/07 21:44:25, Lei Zhang wrote:
> nit: remove

Done.

https://codereview.chromium.org/121383002/diff/670001/media/base/audio_video_...
media/base/audio_video_metadata_extractor.h:14: struct AVDictionary;
On 2014/01/07 21:44:25, Lei Zhang wrote:
> Are you going to use AVDictionary outside of this class? If not, then make it
> private?

Declared by FFmpeg.

https://codereview.chromium.org/121383002/diff/670001/media/base/audio_video_...
media/base/audio_video_metadata_extractor.h:29: bool Extract(media::DataSource*
source);
On 2014/01/07 21:44:25, Lei Zhang wrote:
> nit:: you can omit media:: inside namespace media.

Done.

https://codereview.chromium.org/121383002/diff/670001/media/base/audio_video_...
media/base/audio_video_metadata_extractor.h:31: double duration() const;
On 2014/01/07 21:44:25, Lei Zhang wrote:
> Mention this is in seconds.

Done.

[Lei Zhang, 2014-01-07 23:20:03]

lgtm

https://codereview.chromium.org/121383002/diff/750001/media/base/audio_video_...
File media/base/audio_video_metadata_extractor.cc (right):

https://codereview.chromium.org/121383002/diff/750001/media/base/audio_video_...
media/base/audio_video_metadata_extractor.cc:26: if
(!LowerCaseEqualsASCII(std::string(tag->key), expected_key))
Do you need to convert |tag->key| to a std::string? Ditto in ExtractInt().

[tommycli, 2014-01-07 23:22:28]

thanks!

https://codereview.chromium.org/121383002/diff/750001/media/base/audio_video_...
File media/base/audio_video_metadata_extractor.cc (right):

https://codereview.chromium.org/121383002/diff/750001/media/base/audio_video_...
media/base/audio_video_metadata_extractor.cc:26: if
(!LowerCaseEqualsASCII(std::string(tag->key), expected_key))
On 2014/01/07 23:20:03, Lei Zhang wrote:
> Do you need to convert |tag->key| to a std::string? Ditto in ExtractInt().

Yes, as the char* forms take an explicit start and stop pointer and don't look
for the '\0' character.

[commit-bot: I haz the power, 2014-01-07 23:23:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommycli@chromium.org/121383002/750001

[commit-bot: I haz the power, 2014-01-08 02:16:43]

Change committed as 243470