Open Bug 1016413 Opened 6 years ago Updated 2 years ago

Extracting metadata from files when using the MP4Reader backend

Categories

(Core :: Audio/Video: Playback, defect, P2)

x86
macOS
defect

Tracking

()

People

(Reporter: corentin.cos, Assigned: corentin.cos, Mentored)

References

Details

Attachments

(2 files, 18 obsolete files)

4.90 KB, patch
rillian
: review+
Details | Diff | Splinter Review
579.60 KB, patch
rillian
: review+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/35.0.1916.114 Safari/537.36
Component: Untriaged → Video/Audio
Product: Firefox → Core
Reporting the video metadata (author, copyright, etc.) with MP4Reader backend because it is not done yet
Depends on: 763010
Assignee: nobody → corentin.cos
Hello everyone.
So far, the implementation for this bug extracts the following metadata:
author
artist
album
genre
composer
title
year
author
location
date

Do you have any suggestions of any other info we'd need?

I have another question : so far, for this bug, the keys used in the MetadataTags hash table have explicit names like "artist", "author", etc. Should it be something else ? Things such as kKeyArtist for instance for artist metadata?

Hope to help,
Thanks!
Would it be possible to just get all of them? Like, by looping on all the available metadata? iirc that's the approach we took.

I know there were some people interested in using Dublin core for that, I'm not sure where we are at. Ralph, any clue/advices?
Flags: needinfo?(giles)
So too things. The current media.mozGetMetadata() interface returns a copy of _all_ the current metadata tags. So that would make sense for generic containers. IIRC not all mp4 metadata is that unstructured, but that's a reasonable way to go to get parity for unsupported formats.

Now, several people told me they were unhappy with this generic interface, and that we should have a metadata object with a fixed schema and try to support only those keys. The minimal useful subset we came up with is:

Interface Metadata {
  readonly attribute DOMString creator;
  readonly attribute DOMString title;
  readonly attribute DOMString album;
  readonly attribute Date?     date; // or a integer/DOMString year? union?
  readonly attribute Blob?     artwork;
}

Maybe with an 'change' event on it, or 'metadatachanged' on the parent object.

This really only uses Dublin Core in that we call the artist/author/organization 'creator'. Dublin Core doesn't have an album, and for other reasons isn't a great match to general media player needs.

Implementing something like that as a new proposal is also worthwhile. I couldn't get any interest from other browsers when I tried a couple of years ago, but there's been some now around the DataCue proposal.
Flags: needinfo?(giles)
Thank you both for your answers.
So I see 2 different approaches here. Either putting all the generic tags we find into the MetadataTags structure, or restricting it to a fixed schema.
I’m not sure what you mean by the ‘change’ event or ‘metadatachanged’ on the parent object, though?

It sounds like the first approach is the one taken so far. At the same time, I’m feeling that the use would be leaning toward the second approach, from what you’ve been told. Should this bug also heads toward this second approach? It’s been implemented with the first one (loop over all the metadata) so far - this way a person expecting a specific metadata not specified in the fixed schema wouldn’t be restricted to get it. Not sure it's that relevant in comparison to the need of a fixed schema though.
It might be a detail, but it seems we have distinct tags for both artist and author metadata in MP4 format. In the hypothesis that this bug implements this fixed schema, and that we have both artist and author data in our metadata but containing different data - which one should be chosen as the ‘creator'? The author?

Thank you,

Regards,

Corentin Cos
I propose the following plan:

1) Implement and land the first approach, so mozGetMetadata works everwhere.
2) Implement the second approach behind a pref, if you have time.
3) We'll propose the second approach to whatwg and see if we get anywhere this time.

Step (1) is immediately useful. The hard part of (2) is working out the spec details and getting interest from other browsers. It's good (and fun!) to work on an implementation of (2) as an example for the proposal in step (3), but we won't be able to turn it on immediately. Thus, (1) is worth doing first.
(In reply to Corentin Cos from comment #5)

> I’m not sure what you mean by the ‘change’ event or ‘metadatachanged’ on the
> parent object, though?

This is so web page authors have some way of knowing when the metadata changes. E.g. in a live stream. Right now you have to 'poll' by calling mozGetMetadata periodically to see if the contents changed, but that's a waste. Better to have the decoder tell the media element if it gets new data, and it can queue some event. The page can attach and listener to that event which reloads, redisplays, etc. its tags.

> In the hypothesis that this bug implements
> this fixed schema, and that we have both artist and author data in our
> metadata but containing different data - which one should be chosen as the
> ‘creator'? The author?

That's part of what makes the fixed schema a hard problem; you have to figure out a good mapping for each format and implement that as part of the reader. What is 'author' used for? Is it just for audio books, or do people put the composer in there to distinguish it from a performer in 'artist' tag for music?

Probably we want something like '|Creator| is the value of the author tag, or if it's not present, the value of the artist tag.' The goal of the schema is pretty much what you'd want a player app to display, so think about what's useful there.
So to clarify, I think this bug should be about step (1).

Any work on the fixed schema stuff should go under new bugs. Make those depend on bug 793737.
Looping over all the metadata
(In reply to Ralph Giles (:rillian) from comment #7)
> (In reply to Corentin Cos from comment #5)
> 
> > I’m not sure what you mean by the ‘change’ event or ‘metadatachanged’ on the
> > parent object, though?
> 
> This is so web page authors have some way of knowing when the metadata
> changes. E.g. in a live stream. Right now you have to 'poll' by calling
> mozGetMetadata periodically to see if the contents changed, but that's a
> waste. Better to have the decoder tell the media element if it gets new
> data, and it can queue some event. The page can attach and listener to that
> event which reloads, redisplays, etc. its tags.

Ah, I understand better now. Thanks for the clarification!

> > In the hypothesis that this bug implements
> > this fixed schema, and that we have both artist and author data in our
> > metadata but containing different data - which one should be chosen as the
> > ‘creator'? The author?
> 
> That's part of what makes the fixed schema a hard problem; you have to
> figure out a good mapping for each format and implement that as part of the
> reader. What is 'author' used for? Is it just for audio books, or do people
> put the composer in there to distinguish it from a performer in 'artist' tag
> for music?
> 
> Probably we want something like '|Creator| is the value of the author tag,
> or if it's not present, the value of the artist tag.' The goal of the schema
> is pretty much what you'd want a player app to display, so think about
> what's useful there.

Indeed, I'm guessing that authors and artists are pretty much unlikely to be in the same file metadata. It seems that there would still be some challenges to properly map the info with this schema, hard but interesting problem.
Attachment #8433159 - Attachment is obsolete: true
Attachment #8433155 - Attachment is obsolete: true
Attachment #8434214 - Attachment is obsolete: true
This update should fix issues that happened in the previous version of this patch when building with B2G KK, JB, and ICS Emulator Opt on the Try.
Attachment #8434212 - Attachment is obsolete: true
A few things about this update of the patch :

- In the first place the test for the reporting of MP4 files metadata was done in a single file, apart from test_metadata.html, it's now included in test_metadata.html.

- "type" attributes have been added to the gMetadataTests elements in content/media/test/manifest.js, because in MediaTestManager::nextTest while loop, there is a test with canPlayType, which doesn't work without this "type" attribute specified. It's even more obvious on Linux, when specifically testing metadata reporting with fmp4 backend, and therefore without gstreamer enabled, there are some errors on the java console telling the mp4 format is not supported. Adding the "type" in element of gMetadataTests permits to manage this problem right in the MediaTestManager::nextTest while loop; no error displayed in the console then.

- Some pref have been added in content/media/testtest_metadata.html.
One of this pref aims to disable the use of gstreamer, here is why:
basically, it seems that there are 2 possible codepaths for reporting metadata on Linux. One through GStreamer, one through what has been implemented for this bug.
Nothing has been implemented so far for GStreamer as for reporting metadata. GStreamer also seems to be the default choice to process the mp4 file over fmp4 if both enabled. Therefore, if GStreamer is not disabled, the tests for mp4 files implemented in test_metadata.html fail because it uses GStreamer backend, which is not ready. Here is the first reason for the setting of this pref. Then, this test aims to specifically test the correct reporting of metadata through the fmp4 backend. With the "type" attribute I added to the tested elements I spoke about above, Linux will simply skip the mp4 files it wouldn't be able to process with fmp4 backend, without any fail or error. Well, here comes the question as how would it be managed if in a near future GStreamer codepaths for parsing/reporting metadata is implemented and up, as well as the fmp4 one with mp4_demuxer, in term of testing metadata reporting on mp4 files ? There'd be 2 codepaths to do so with mp4 files, and I'm not sure how it could be done. I heard that mp4_demuxer is supposed to take GStreamer's place in a near future anyway.

- I made some slight code refactorization in test_metadata.html, what do you guys think of it ? I simply moved the function from the EventListener arguments somewhere else, with a more explicit name.

I hope I have been clear, hope to help!
Attachment #8436005 - Attachment is obsolete: true
Comment on attachment 8436348 [details] [diff] [review]
Bug 1016413 - Implementation for reporting metadata from MP4 files.

Review of attachment 8436348 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/libstagefright/binding/mp4_demuxer.cpp
@@ +82,5 @@
>  
> +static nsCString
> +ToCString(uint32_t value)
> +{
> +  char strValue[5];

nsCString s;
s.AppendInt(value);
return s;
Comment on attachment 8436352 [details] [diff] [review]
Bug 1016413 - Tests for the implementation for reporting metadata from MP4 files.

Review of attachment 8436352 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/test/manifest.js
@@ +485,5 @@
>  // These are files with non-trivial tag sets.
>  // Used by test_metadata.html.
>  var gMetadataTests = [
>    // Ogg Vorbis files
> +  { name:"short-video.ogv", 

Please don't add trailing whitespace, here and elsewhere.

::: content/media/test/test_metadata.html
@@ +89,5 @@
> +SpecialPowers.pushPrefEnv({
> +        'set': [
> +      ["media.fragmented-mp4.ffmpeg.enabled", true],
> +      ["media.fragmented-mp4.exposed", true],
> +      ["media.gstreamer.enabled", false]

Instead of disabling gstreamer, we should make the MP4Reader instantiated in preference to the GStreamer backend.

So change DecoderTraits::CreateReader() [1] and DecoderTraits::InstantiateDecoder() [2] so that the MOZ_FMP4 case appears before all others.

[1] http://mxr.mozilla.org/mozilla-central/source/content/media/DecoderTraits.cpp#596
[2] http://mxr.mozilla.org/mozilla-central/source/content/media/DecoderTraits.cpp#486
(In reply to Chris Pearce (:cpearce) from comment #18)
> Comment on attachment 8436348 [details] [diff] [review]
> Bug 1016413 - Implementation for reporting metadata from MP4 files.
> 
> Review of attachment 8436348 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/libstagefright/binding/mp4_demuxer.cpp
> @@ +82,5 @@
> >  
> > +static nsCString
> > +ToCString(uint32_t value)
> > +{
> > +  char strValue[5];
> 
> nsCString s;
> s.AppendInt(value);
> return s;

Thank you for your feedback. Actually, what was done here with the bit shifting thing, was, from a certain integer on 32 bytes, we'd get the 4 characters corresponding to each 8 bytes pack. With AppendInt, it just returns the integer, but in a String type. 
Example:
we have a certain key == kKeyArtist. 
- With the bit shifting calculation, the nsCString returned would be "arti". (basically it gets back the key stored in media/libstagefright/frameworks/av/include/MetaData.h in char* type)
- With AppendInt, the nsCString returned would be "1634890857".
This nsCString key is then stored in the MetadataTags structure.

Maybe we'd rather report the tag value in integer format?
(In reply to Chris Pearce (:cpearce) from comment #19)
> Comment on attachment 8436352 [details] [diff] [review]
> Bug 1016413 - Tests for the implementation for reporting metadata from MP4
> files.
> 
> Review of attachment 8436352 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> 
> ::: content/media/test/test_metadata.html
> @@ +89,5 @@
> > +SpecialPowers.pushPrefEnv({
> > +        'set': [
> > +      ["media.fragmented-mp4.ffmpeg.enabled", true],
> > +      ["media.fragmented-mp4.exposed", true],
> > +      ["media.gstreamer.enabled", false]
> 
> Instead of disabling gstreamer, we should make the MP4Reader instantiated in
> preference to the GStreamer backend.
> 
> So change DecoderTraits::CreateReader() [1] and
> DecoderTraits::InstantiateDecoder() [2] so that the MOZ_FMP4 case appears
> before all others.
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/content/media/DecoderTraits.
> cpp#596
> [2]
> http://mxr.mozilla.org/mozilla-central/source/content/media/DecoderTraits.
> cpp#486

OK, will do. Do we mind about letting the other prefs set here ? (AKA "media.fragmented-mp4.ffmpeg.enabled", "media.fragmented-mp4.exposed") Or should we directly modify them for instance in module/libpref/src/init/all.js - and include this in the patch -, where it seems to be statically set?
(In reply to Ralph Giles (:rillian) from comment #20)
> https://tbpl.mozilla.org/?tree=Try&rev=42b42d2d6f11

Thank you for the push to Try Ralph Giles ! This time it looks pretty green :-)
(In reply to Corentin Cos from comment #21)
> (In reply to Chris Pearce (:cpearce) from comment #18)
> > Comment on attachment 8436348 [details] [diff] [review]
> > Bug 1016413 - Implementation for reporting metadata from MP4 files.
> > 
> > Review of attachment 8436348 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: media/libstagefright/binding/mp4_demuxer.cpp
> > @@ +82,5 @@
> > >  
> > > +static nsCString
> > > +ToCString(uint32_t value)
> > > +{
> > > +  char strValue[5];
> > 
> > nsCString s;
> > s.AppendInt(value);
> > return s;
> 
> Thank you for your feedback. Actually, what was done here with the bit
> shifting thing, was, from a certain integer on 32 bytes, we'd get the 4
> characters corresponding to each 8 bytes pack.

Ah, I see. What you're doing is fine then.


(In reply to Corentin Cos from comment #22)
> OK, will do. Do we mind about letting the other prefs set here ? (AKA
> "media.fragmented-mp4.ffmpeg.enabled", "media.fragmented-mp4.exposed") Or
> should we directly modify them for instance in
> module/libpref/src/init/all.js - and include this in the patch -, where it
> seems to be statically set?

We're not ready to set those other prefs on by default. That would turn this on for all platforms, and this code is no where near stable enough to expose to the web yet!
Update from the last patch :
- Changed the preference of FMP4 in DecoderTraits by setting its case before GStreamer's
- Added a comment for the ToCString function to explain what it aims to do
Attachment #8436348 - Attachment is obsolete: true
update for the last test patch :
- Added a skip-if condition for the test_metadata.html for it not be launched on B2G emulator, in mochitest.ini (or it fails because it seems it's omx codepaths used for MP4 files data extraction in B2G ICS - and it might not report metadata yet). This skip-if test would be meant to be removed as soon as possible. Isn't it too restrictive?
Attachment #8436352 - Attachment is obsolete: true
(In reply to Chris Pearce (:cpearce) from comment #24)
> (In reply to Corentin Cos from comment #22)
> > OK, will do. Do we mind about letting the other prefs set here ? (AKA
> > "media.fragmented-mp4.ffmpeg.enabled", "media.fragmented-mp4.exposed") Or
> > should we directly modify them for instance in
> > module/libpref/src/init/all.js - and include this in the patch -, where it
> > seems to be statically set?
> 
> We're not ready to set those other prefs on by default. That would turn this
> on for all platforms, and this code is no where near stable enough to expose
> to the web yet!

So after some tests with or without the media.gstreamer.enabled pref set to false, it seems that there is a memory leak (148 bytes) if media.fragmented-mp4.ffmpeg.enabled pref is set to true and that media.gstreamer.enabled is not explicitly set to false at the same time, despite the change of case orders between GStreamer and FMP4 in DecoderTraits::InstantiateDecoder and DecoderTraits::CreateReader as you suggested. This leak even happens if I let |*aTags = nullptr;| in MP4Reader::readMetadata.

Also, the test on the mp4 metadata extraction (content/media/test/test_metadata.html) doesn't give the same result whether media.gstreamer.enabled pref is explicitly set to false or not (with the 2 other pref sets in the test):
- If it is set to false, canPlayType for mp4 type returns false in my Linux environment - therefore the tests on mp4 metadata reporting are skipped in the MediaTestManager::nextTest loop (a type attribute has been added to each element of the gMetadataTests structure so that this canPlayType test is not based on undefined values and works correctly) -> no leak at all then;

- If it is set to false, but that I force the test to happen, the mp4 metadata is correctly reported and there is the leak (I can even get an error message in the console like "Invalid URI. Load of media resource failed");

- If the pref is not set to false or anything, canPlayType for mp4 returns true, and the mp4 metadata is reported -> but we have our 148 bytes leak at the end.

I'm actually pretty confused by these results to be honest. I believe there would be some conflicts, with gstreamer and ffmpeg or something, but I'm not sure I understand all of this so far. 
The thing is I believe I need media.fragmented-mp4.ffmpeg.enabled set to true to correctly process MP4 files with fmp4 backend. Should I leave the setting of the pref media.gstreamer.enabled to false then? It's likely I misunderstood something with these different interactions (it didn't sound like media.fragmented-mp4.ffmpeg.enabled should be set, in the first place - maybe I'm mistaking somewhere to have the need to have it set to true)

----
Here is this memory leak :
INFO -  TEST-INFO | leakcheck | leaked 1 Image (52 bytes)
INFO -  TEST-INFO | leakcheck | leaked 1 Mutex (12 bytes)
INFO -  TEST-INFO | leakcheck | leaked 1 RecycleBin (24 bytes)
INFO -  TEST-INFO | leakcheck | leaked 1 VideoData (56 bytes)
INFO -  TEST-INFO | leakcheck | leaked 1 nsTArray_base (4 bytes)
WARNING -  TEST-UNEXPECTED-FAIL | leakcheck | 148 bytes leaked (Image, Mutex, RecycleBin, VideoData, nsTArray_base)
----
When it's been repro on the Try, when I click on "Analyze the leak", in the blue box, I get the following message :
No DOMWINDOWs leaked!

Thank you for your help !
Comment on attachment 8437673 [details] [diff] [review]
BUG 1016413 : Implementation for reporting metadata from mp4 files with fmp4 backend.

Review of attachment 8437673 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/DecoderTraits.cpp
@@ +487,5 @@
>  {
>    nsRefPtr<MediaDecoder> decoder;
>  
> +#ifdef MOZ_FMP4
> +  // Note: fmp4 should come before GStreamer, 

Don't add trailing space.

If you run `./mach clang-format`, it should fix these things.

@@ +600,5 @@
>    MediaDecoderReader* decoderReader = nullptr;
>  
> +#ifdef MOZ_FMP4
> +  // Note: MP4Reader is preferred for MP4, but if it's disabled we
> +  // fallback to the GStreamerReader.

This comment is only true on Linux. Remember we also run this same code on Windows, Mac, Android, and FirefoxOS.

Please remove this comment, or fix it.

::: media/libstagefright/binding/mp4_demuxer.cpp
@@ +103,5 @@
> +  uint32_t key = -1;
> +
> +  for (size_t i = 0; i < metaData->size(); i++) {
> +    key = metaData->keyAt(i);
> +    if (metaData->findCString(key, &value))

Always put curly braces {} on one line if statements.

::: media/libstagefright/frameworks/av/include/media/stagefright/MetaData.h
@@ +216,5 @@
>                    const void **data, size_t *size) const;
>  
>      void dumpToLog() const;
>  
> +    size_t size();

"size" of what? The number of bytes this struct is allocated? A better name would be "numKeys()".
Comment on attachment 8437693 [details] [diff] [review]
bug 1016413 : tests for reporting metadata from mp4 files with fmp4 backend.

Review of attachment 8437693 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/test/mochitest.ini
@@ +384,5 @@
>  [test_mediarecorder_record_stopms.html]
>  [test_mediarecorder_record_timeslice.html]
>  [test_mediarecorder_reload_crash.html]
>  [test_mediarecorder_unsupported_src.html]
>  [test_metadata.html]

You're skipping test_mixed_principals here on gonk, not test_metadata.

Regardless, we still actually want to test that the ogg metadata is working, so don't do this.

Just determine the platform (you can get his from the navigator.userAgent string) in test_metadata iteself, and don't run the mp4 metadata tests on platforms other than Windows 7 and later, and Linux.
(In reply to Corentin Cos from comment #27)
> (In reply to Chris Pearce (:cpearce) from comment #24)
> > (In reply to Corentin Cos from comment #22)
> > > OK, will do. Do we mind about letting the other prefs set here ? (AKA
> > > "media.fragmented-mp4.ffmpeg.enabled", "media.fragmented-mp4.exposed") Or
> > > should we directly modify them for instance in
> > > module/libpref/src/init/all.js - and include this in the patch -, where it
> > > seems to be statically set?
> > 
> > We're not ready to set those other prefs on by default. That would turn this
> > on for all platforms, and this code is no where near stable enough to expose
> > to the web yet!
> 
> So after some tests with or without the media.gstreamer.enabled pref set to
> false, it seems that there is a memory leak (148 bytes)

It's because we have unknown problems like this that we're not shipping the MP4Reader preffed on yet. ;)

This is bug 1022499. It's being worked on.


> Also, the test on the mp4 metadata extraction
> (content/media/test/test_metadata.html) doesn't give the same result whether
> media.gstreamer.enabled pref is explicitly set to false or not (with the 2
> other pref sets in the test):

GStreamer doesn't implement metadata extraction yet. Metadata extraction is only implemented for Ogg so far.


> - If it is set to false, canPlayType for mp4 type returns false in my Linux
> environment

MP4Decoder::GetSupportedCodecs() should return true for "video/mp4":
http://mxr.mozilla.org/mozilla-central/source/content/media/fmp4/MP4Decoder.cpp#64

Why is this not being called?
Updates from the last version of the patch :
- no more skip-if test in content/media/test/mochitest.ini, there is now a test made in content/media/test/test_metadata.html that specifically restricts the test for metadata reporting on mp4 files to some platforms (Windows 7 and 8, Linux);
- There is a leak when launching the tests, it's actually a bug (https://bugzilla.mozilla.org/show_bug.cgi?id=1022499) that is being worked on (thank you Chris Pearce for letting me know about it)
Attachment #8437693 - Attachment is obsolete: true
Updates compared to the previous version :
- s/MetaData::size/MetaData::numKeys/
- added braces for an if statement in MP4Demuxer::GetTags
- Removed some ambiguous comments in DecoderTraits::InstantiateDecoder and DecoderTraits::CreateReader
Attachment #8437673 - Attachment is obsolete: true
Thank you very much for your feedback Chris Pearce !

(In reply to Chris Pearce (:cpearce) from comment #30)
> (In reply to Corentin Cos from comment #27)
> > (In reply to Chris Pearce (:cpearce) from comment #24)
> > 
> > So after some tests with or without the media.gstreamer.enabled pref set to
> > false, it seems that there is a memory leak (148 bytes)
> 
> It's because we have unknown problems like this that we're not shipping the
> MP4Reader preffed on yet. ;)
> 
> This is bug 1022499. It's being worked on.
> 

Ah, thank you for letting me know. I guess we should still let the mp4 metadata tests in this patch despite this "temporary" leak bug?

> > - If it is set to false, canPlayType for mp4 type returns false in my Linux
> > environment
> 
> MP4Decoder::GetSupportedCodecs() should return true for "video/mp4":
> http://mxr.mozilla.org/mozilla-central/source/content/media/fmp4/MP4Decoder.
> cpp#64
> 
> Why is this not being called?

In manifest.js, in the method MediaTestManager::nextTest's while loop, there is a test on the type of the current element, and whether it's a supported video type on the navigator with canPlayType: 
-------
// Ensure we can play the resource type.
if (test.type && !document.createElement('video').canPlayType(test.type))
  continue;
-------
the canPlayType test there for gizmo.metadata.mp4 returns false, and so it ignores its test. Is MP4Decoder::GetSupportedCodecs() called during the canPlayType() test or after ? So far, tests have shown that there were no tracks of the mp4 file in MP4Decoder::GetSupportedCodecs() when canPlayType returns false.
Attachment #8438493 - Flags: review?(cpearce)
Attachment #8438488 - Flags: review?(cpearce)
Comment on attachment 8438488 [details] [diff] [review]
bug 1016413: tests for reporting metadata from mp4 files with fmp4 backend.

Review of attachment 8438488 [details] [diff] [review]:
-----------------------------------------------------------------

Ralph Giles will take over the review of all things metadata. He's previously worked on metadata fetching.
Attachment #8438488 - Flags: review?(cpearce) → review?(giles)
Comment on attachment 8438493 [details] [diff] [review]
bug 1016413: implementation for reporting metadata from mp4 files with fmp4 backend.

Review of attachment 8438493 [details] [diff] [review]:
-----------------------------------------------------------------

Ralph Giles will take over the review of these. He's previously worked on metadata fetching.
Attachment #8438493 - Flags: review?(cpearce) → review?(giles)
Comment on attachment 8438493 [details] [diff] [review]
bug 1016413: implementation for reporting metadata from mp4 files with fmp4 backend.

Review of attachment 8438493 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay. I think this is getting close, but I have one significant request. Please address these comments and ask for review again.

In talking about this work with Chris, I was reminded of the patch I did for bug 778052. That patch also pulled data out of the stagefright demuxer, but for the platform version on Android. In it I make some attempt at normalization with the tag names returned by the Ogg parsers. I do so by enumerating defined keys rather than returning the raw fourcc codes like your patch does. We'd like you to use that approach for this patch as well.

I abandoned my patch because of the consistency issues: what stagefright was returning wasn't really 'mp4' tags, but its own remapping from whatever it parsed out of the underlying file. This was a problem at the time because we wouldn't just be returning different raw tag names for different file types, but different tag names for the _same_ file on _different_ platforms. That way lies yuck.

We now intend to standardize on the fmp4 demuxer for all handling of mp4 files, so that's not an issue with your patch. But if stagefright still isn't returning the keys which are really in the mp4 file (I didn't verify this; you might want to check) we might as well advance our plans for a standard mapping. There's nothing 'honest' about just enumerating the fourcc codes.

Let me know if you have any questions.

::: media/libstagefright/binding/include/mp4_demuxer/mp4_demuxer.h
@@ +14,5 @@
>  {
>  
>  struct StageFrightPrivate;
>  typedef int64_t Microseconds;
> +typedef mozilla::MetadataTags MetadataTags;

Please use an explicit namespace prefix for mozilla::MetadataTags rather than a typedef. There aren't many references and it matches the mozilla::Vector references in DecoderData.
Attachment #8438493 - Flags: review?(giles) → review-
Updates from the previous version of the patch for the implementation:
- no more typedef for MetadataTags in media/libstagefright/binding/include/mp4_demuxer/mp4_demuxer.h;
- Now a mapping between keys and its normalized char* name.
Attachment #8438493 - Attachment is obsolete: true
Attachment #8439807 - Flags: review?(giles)
Update of the tests because of the change for normalized tag name values in the implementation.
Attachment #8438488 - Attachment is obsolete: true
Attachment #8438488 - Flags: review?(giles)
Attachment #8439811 - Flags: review?(giles)
(In reply to Ralph Giles (:rillian) from comment #37)
> Comment on attachment 8438493 [details] [diff] [review]
> bug 1016413: implementation for reporting metadata from mp4 files with fmp4
> backend.
> 
> Review of attachment 8438493 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for the delay. I think this is getting close, but I have one
> significant request. Please address these comments and ask for review again.
> 
> In talking about this work with Chris, I was reminded of the patch I did for
> bug 778052. That patch also pulled data out of the stagefright demuxer, but
> for the platform version on Android. In it I make some attempt at
> normalization with the tag names returned by the Ogg parsers. I do so by
> enumerating defined keys rather than returning the raw fourcc codes like
> your patch does. We'd like you to use that approach for this patch as well.
> 
> We now intend to standardize on the fmp4 demuxer for all handling of mp4
> files, so that's not an issue with your patch. But if stagefright still
> isn't returning the keys which are really in the mp4 file (I didn't verify
> this; you might want to check) we might as well advance our plans for a
> standard mapping. There's nothing 'honest' about just enumerating the fourcc
> codes.
> 
> Let me know if you have any questions.

Thank you very much for your review.
A mapping structure between the keys and normalized/standardized values has been added in the mp4_demuxer and is now used for putting information into MetadataTags - I hope it fits with your request.
(In reply to Ralph Giles (:rillian) from comment #41)
> https://tbpl.mozilla.org/?tree=Try&rev=c17ac8c34fcd

Thank you for the push to Try Ralph Giles, all green but some intermittents :-)
Comment on attachment 8439807 [details] [diff] [review]
bug 1016413: implementation for reporting metadata from mp4 files with fmp4 backend.

Review of attachment 8439807 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for adding the mapping table. I'd like to see it again after addressing comments.

::: media/libstagefright/binding/mp4_demuxer.cpp
@@ +110,5 @@
> +{
> +  mozilla::MetadataTags* tags = new mozilla::MetadataTags;
> +  sp<MetaData> metaData = mPrivate->mExtractor->getMetaData();
> +  const char* value = nullptr;
> +  uint32_t key_count = sizeof(keys) / sizeof(keys[0]);

Please use

  size_t key_count = mozilla::ArraySize(keys);

instead. This function is in mfbt/ArrayUtils.h and didn't exist when I wrote my patch.

@@ +113,5 @@
> +  const char* value = nullptr;
> +  uint32_t key_count = sizeof(keys) / sizeof(keys[0]);
> +
> +  for (size_t i = 0; i < key_count; i++) {
> +    if (metaData->findCString(keys[i].key, &value)) {

Please validate at least that the value strings are valid utf-8 before calling Put().

@@ +114,5 @@
> +  uint32_t key_count = sizeof(keys) / sizeof(keys[0]);
> +
> +  for (size_t i = 0; i < key_count; i++) {
> +    if (metaData->findCString(keys[i].key, &value)) {
> +      tags->Put(nsCString(strdup(keys[i].name)), nsCString(strdup(value)));

I'm pretty sure this leaks. If I traced the code correctly, this nsCString ctor will call nsTSubstring_CharT::Assign with aLength=-1, which will copy the data. So you shouldn't need the strdup?

IIRC Put() takes ownership, so construcing new nsStrings is the correct thing to do.

(see http://dxr.mozilla.org/mozilla-central/source/xpcom/string/src/nsTSubstring.cpp#299 )

::: media/libstagefright/frameworks/av/include/media/stagefright/MetaData.h
@@ +219,5 @@
>  
> +    size_t numKeys();
> +
> +    uint32_t keyAt(size_t index);
> +

These changes are no longer necessary, right?
Attachment #8439807 - Flags: review?(giles)
Thank you very much for your feedback rillian.
Updates for this new patch come from your remarks :
- Indeed, MetaData::numKeys and MetaData::keyAt got useless now with the mapping;
- it now uses ArrayLength (it's the only size related method in ArrayUtil that I found - no track ArraySize, I guess you meant ArrayLength?);
- removed strdup;
- Validating the value is UTF8 and not void before putting it into the MetadataTags table.
No update for the tests.
Attachment #8439807 - Attachment is obsolete: true
Attachment #8440925 - Flags: review?(giles)
Sorry for the immediate spam - small improvement with CString value.
Attachment #8440925 - Attachment is obsolete: true
Attachment #8440925 - Flags: review?(giles)
Attachment #8440928 - Flags: review?(giles)
Comment on attachment 8439811 [details] [diff] [review]
bug 1016413: tests for reporting metadata from mp4 files with fmp4 backend.

Review of attachment 8439811 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nits addressed.

I'd like to see more than one test, particularly utf-8 validation, but I'm also pretty sure album art won't work with the current patch. But we have enough scope creep as it is. Please file a follow-up bug for those and anything else you can think of testing (aac file?) and let's get this bit landed.

::: content/media/test/manifest.js
@@ +533,5 @@
>        title:"Pew SFX"
>      }
>    },
> +  { name:"badtags.ogg",
> +    type:"video/ogg",

badtags.ogg should be "audio/ogg"

::: content/media/test/test_metadata.html
@@ +80,5 @@
> +
> +  a.src = test.name;
> +  a.name = test.name;
> +  a.value = test;
> +  

trailing whitespace

@@ +102,5 @@
> +SimpleTest.waitForExplicitFinish();
> +SpecialPowers.pushPrefEnv({
> +        'set': [
> +      ["media.fragmented-mp4.ffmpeg.enabled", true],
> +      ["media.fragmented-mp4.exposed", true]

Please move this up so it's next to the definition of MP4isNotTested() so all the conditions are in the same place. Might as well move them both above checkTags() as well, to the top of the file while you're at it.

@@ +105,5 @@
> +      ["media.fragmented-mp4.ffmpeg.enabled", true],
> +      ["media.fragmented-mp4.exposed", true]
> +    ]
> +}, beginTest);
> +  

trailing whitespace
Attachment #8439811 - Flags: review?(giles) → review+
Comment on attachment 8440928 [details] [diff] [review]
bug 1016413: Implementation for reporting mp4 metadata with fmp4 backend

Review of attachment 8440928 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nit addressed.

I like how short this patch is getting!

::: media/libstagefright/binding/mp4_demuxer.cpp
@@ +115,5 @@
> +
> +  for (size_t i = 0; i < key_count; i++) {
> +    if (metaData->findCString(keys[i].key, &value)) {
> +      cstringValue = nsCString(value);
> +      if (IsUTF8(cstringValue) || !cstringValue.IsVoid()) {

You want && here. IsUTF8 doesn't seem to check for null references, so !IsVoid() should be first.

OTOH, it seems like the nsCString points itself at a static empty buffer when it's voided, so maybe the check is meaningless. If you prefer, you could just

  MOZ_ASSERT(value);

before creating cstringValue.
Attachment #8440928 - Flags: review?(giles) → review+
Thank you for your feedback on the test. Building up a bigger set of tests for MP4 format would indeed be a great idea.
Update for the patch of this test in the meantime :
- all the MP4 related conditions (pref, IsMP4 and MP4isNotTested) got moved all above in test_metadata.html;
- Fix on badtags.ogg type.
Attachment #8439811 - Attachment is obsolete: true
Attachment #8440983 - Flags: review?(giles)
Thank you for your fast feedback.
Update of the patch - in this case an isVoid() seems indeed not to be that sensible -> switched to a MOZ_ASSERT(value) like you suggested.
Attachment #8440928 - Attachment is obsolete: true
Attachment #8441003 - Flags: review?(giles)
Comment on attachment 8441003 [details] [diff] [review]
bug 1016413: implementation for reporting mp4 metadata with fmp4 backend

Review of attachment 8441003 [details] [diff] [review]:
-----------------------------------------------------------------

thanks!
Attachment #8441003 - Flags: review?(giles) → review+
Comment on attachment 8440983 [details] [diff] [review]
bug 1016413: Test for the implementation of reporting mp4 metadata with fmp4 backend

Review of attachment 8440983 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.
Attachment #8440983 - Flags: review?(giles) → review+
Comment on attachment 8441003 [details] [diff] [review]
bug 1016413: implementation for reporting mp4 metadata with fmp4 backend

Review of attachment 8441003 [details] [diff] [review]:
-----------------------------------------------------------------

I apologise for the drive-by here.

::: content/media/DecoderTraits.cpp
@@ +491,5 @@
> +  if (IsMP4SupportedType(aType)) {
> +    decoder = new MP4Decoder();
> +    return decoder.forget();
> +  }
> +#endif

These DecoderTraits changes shouldn't be part of this patch.

::: media/libstagefright/binding/mp4_demuxer.cpp
@@ +106,5 @@
>  
> +mozilla::MetadataTags*
> +MP4Demuxer::GetTags()
> +{
> +  mozilla::MetadataTags* tags = new mozilla::MetadataTags;

We need to do a better job of making sure we don't leak memory here. One suggestion would be to add `mozilla::MetadataTags mTags` as a field of MP4Demuxer make GetTags() return a const reference to it.
Thank you very much for your feedback Anthony Jones.

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #52)
> Comment on attachment 8441003 [details] [diff] [review]
> bug 1016413: implementation for reporting mp4 metadata with fmp4 backend
> 
> Review of attachment 8441003 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I apologise for the drive-by here.
> 
> ::: content/media/DecoderTraits.cpp
> @@ +491,5 @@
> > +  if (IsMP4SupportedType(aType)) {
> > +    decoder = new MP4Decoder();
> > +    return decoder.forget();
> > +  }
> > +#endif
> 
> These DecoderTraits changes shouldn't be part of this patch.
> 

Should I open a new bug then, and include the changes on DecoderTraits as a unique patch for this bug?

> ::: media/libstagefright/binding/mp4_demuxer.cpp
> @@ +106,5 @@
> >  
> > +mozilla::MetadataTags*
> > +MP4Demuxer::GetTags()
> > +{
> > +  mozilla::MetadataTags* tags = new mozilla::MetadataTags;
> 
> We need to do a better job of making sure we don't leak memory here. One
> suggestion would be to add `mozilla::MetadataTags mTags` as a field of
> MP4Demuxer make GetTags() return a const reference to it.

For this implementation, I was inspired by the OggCodecState GetTags ones (OpusState::GetTags() http://dxr.mozilla.org/mozilla-central/source/content/media/ogg/OggCodecState.cpp#915 and VorbisState::GetTags() http://dxr.mozilla.org/mozilla-central/source/content/media/ogg/OggCodecState.cpp#645).
Since it is done the same way as what I did, should we consider this leak memory potential issue for the GetTags implementations in OggCodecState as well?

Thank you !
(In reply to Corentin Cos from comment #53)

> > These DecoderTraits changes shouldn't be part of this patch.
> 
> Should I open a new bug then, and include the changes on DecoderTraits as a
> unique patch for this bug?

Yes. The most proper thing to do is to open a new bug, split that part of the patch into a separate attachment there and make this bug depend on that bug.

If we're doing another round, now's a good time to practice you commit message writing. There's a standard form you can see if you look at the commit log:

  Bug 1016413 - Implement mozGetMetadata for fmp4. r=rillian
  [blank line]
  More detailed description of why the change was made. Anything
  which might be helpful to someone trying to figure out why the
  code is the way it is later. Can be brief or verbose depending
  on the nature of the change.
(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #52)

> > +  mozilla::MetadataTags* tags = new mozilla::MetadataTags;
> 
> We need to do a better job of making sure we don't leak memory here. One
> suggestion would be to add `mozilla::MetadataTags mTags` as a field of
> MP4Demuxer make GetTags() return a const reference to it.

No. The tags hashmap can be returned to js, and can thus outlive the decoder: returning a const reference won't work. Rather, all the callers would have to deep-clone it; it seemed simpler to have GetTags() create the object and the caller take ownership.

The caller ends up stashing the outparam in an nsAutoPtr, either in HTMLMediaElement or in MetadataManager, so this doesn't leak. I agree it's a little ugly. Anthony, if you'd like to change the pattern, please file a new bug and assign it to Corentin. This patch copies existing code, which is the appropriate thing to do.

mozilla::MetadataTags is an nsDataHashtable, which isn't a refcounted class, so we'd have to wrap it to make the ownership transfer explicit by returning an already_AddRefed. Could return an nsAutoPtr<>& maybe?
Depends on: 1026704
Update for this new version of the patch :
- No more DecoderTraits change of implementation on this patch, it is now done on bug 1026704 (this bug now depends on bug 1026704).
Attachment #8441003 - Attachment is obsolete: true
Attachment #8441625 - Flags: review?(giles)
(In reply to Ralph Giles (:rillian) from comment #54)
> (In reply to Corentin Cos from comment #53)
> 
> > > These DecoderTraits changes shouldn't be part of this patch.
> > 
> > Should I open a new bug then, and include the changes on DecoderTraits as a
> > unique patch for this bug?
> 
> Yes. The most proper thing to do is to open a new bug, split that part of
> the patch into a separate attachment there and make this bug depend on that
> bug.
> 
> If we're doing another round, now's a good time to practice you commit
> message writing. There's a standard form you can see if you look at the
> commit log:
> 
>   Bug 1016413 - Implement mozGetMetadata for fmp4. r=rillian
>   [blank line]
>   More detailed description of why the change was made. Anything
>   which might be helpful to someone trying to figure out why the
>   code is the way it is later. Can be brief or verbose depending
>   on the nature of the change.

A new bug has now been opened (bug 1027604) - some improvements have also been made on commit messages, following your recommendations. Thanks!
Comment on attachment 8441625 [details] [diff] [review]
bug 1016413: implementation for reporting mp4 metadata with fmp4 backend

Review of attachment 8441625 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with commit message nit addressed.

Capitalize 'Bug' in the commit message and remove the redundant first 'mp4'.
Attachment #8441625 - Flags: review?(giles) → review+
Update the patch with commit message improvements, addressing Ralph's previous comment.
Attachment #8441625 - Attachment is obsolete: true
Attachment #8441666 - Flags: review?(giles)
Update of the commit message for the patch test related.
Attachment #8440983 - Attachment is obsolete: true
Attachment #8441669 - Flags: review?(giles)
Comment on attachment 8441666 [details] [diff] [review]
bug 1016413: implementation for reporting metadata with fmp4 backend

Review of attachment 8441666 [details] [diff] [review]:
-----------------------------------------------------------------

Dash (ASCII '-' character) after the bug number, not a colon.
Attachment #8441666 - Flags: review?(giles) → review+
Comment on attachment 8441669 [details] [diff] [review]
bug1016413: test for the implementation of reporting mp4 metadata with fmp4 backend

Review of attachment 8441669 [details] [diff] [review]:
-----------------------------------------------------------------

Likewise.

I can fix that and push to inbound if you like.
Attachment #8441669 - Flags: review?(giles) → review+
(In reply to Ralph Giles (:rillian) from comment #62)
> Comment on attachment 8441669 [details] [diff] [review]
> bug1016413: test for the implementation of reporting mp4 metadata with fmp4
> backend
> 
> Review of attachment 8441669 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Likewise.
> 
> I can fix that and push to inbound if you like.

Sure, this would be great, thank you very much !
Inbound is closed, so pushing to try on top of m-c: https://tbpl.mozilla.org/?tree=Try&rev=4df0d65d15f8
Looks like we've got a new leak on Linux debug mochitests. Corentin, can you try to narrow this down to a particular test?
From https://tbpl.mozilla.org/php/getParsedLog.php?id=41982833&tree=Try

12:08:02     INFO -  nsTraceRefcnt::DumpStatistics: 1703 entries
12:08:02     INFO -  TEST-INFO | leakcheck | leaked 1 Image (96 bytes)
12:08:02     INFO -  TEST-INFO | leakcheck | leaked 1 Mutex (24 bytes)
12:08:02     INFO -  TEST-INFO | leakcheck | leaked 1 RecycleBin (48 bytes)
12:08:02     INFO -  TEST-INFO | leakcheck | leaked 1 VideoData (80 bytes)
12:08:02     INFO -  TEST-INFO | leakcheck | leaked 1 nsTArray_base (8 bytes)
12:08:02  WARNING -  TEST-UNEXPECTED-FAIL | leakcheck | 256 bytes leaked (Image, Mutex, RecycleBin, VideoData, nsTArray_base)
(In reply to Ralph Giles (:rillian) from comment #65)
> From https://tbpl.mozilla.org/php/getParsedLog.php?id=41982833&tree=Try
> 
> 12:08:02     INFO -  nsTraceRefcnt::DumpStatistics: 1703 entries
> 12:08:02     INFO -  TEST-INFO | leakcheck | leaked 1 Image (96 bytes)
> 12:08:02     INFO -  TEST-INFO | leakcheck | leaked 1 Mutex (24 bytes)
> 12:08:02     INFO -  TEST-INFO | leakcheck | leaked 1 RecycleBin (48 bytes)
> 12:08:02     INFO -  TEST-INFO | leakcheck | leaked 1 VideoData (80 bytes)
> 12:08:02     INFO -  TEST-INFO | leakcheck | leaked 1 nsTArray_base (8 bytes)
> 12:08:02  WARNING -  TEST-UNEXPECTED-FAIL | leakcheck | 256 bytes leaked
> (Image, Mutex, RecycleBin, VideoData, nsTArray_base)
>
> Looks like we've got a new leak on Linux debug mochitests. Corentin, can you
> try to narrow this down to a particular test?

I had made some tests around this bug and the results were mysterious (see https://bugzilla.mozilla.org/show_bug.cgi?id=1016413#c27) - it's actually bug 1022499 and being worked on.
Ok, great, the leaks aren't new with these patches. Looks like this is ready to land.
Keywords: checkin-needed
Corentin:

I can reproduce with './mach mochitest-plain --e10s content/media/test/test_metadata.html'

First the test fails on gizmo.metadata.mp4 because no tags are returned. The test then eventually times out, presumedly because manager.finished() is never called.
Sorry, my local version of the test fails with and without --e10s because I don't have ffmpeg installed. I can't reproduce the crash.

If only canPlayType() was useful for disabling the test in the no-ffmpeg case.
I can't reproduce the crash with --e10s as well. Quite mysterious, I'm going to look into it.
As for the leak, it is worked on in the bug 1022499 - Maybe we should rather wait for this to be fixed before landing the patch?
Mentor: cpearce, giles, paul
Sorry guys I’ve been quite busy lately. If there are no tags reported (« no output ») it is likely because GStreamer backend is used - if there was any problem with ffmpeg I believe there would be undefined values rather than no tags returned (which would cause another kind of failure).
... But when I look more carefully into the logs, I see tracks of MP4Demuxer, MP4Reader, as well as ffmpeg. Pretty confusing.
What does —e10s do ? What is it used for ? I’ve seen threads in the logs, and I found a few resources telling e10s had something to do with multiprocess - but I might miss some info about this, because I’m not sure how using different threads could end up in this fail.
Thanks !
In e10s each tab runs in a separate process. This is currently an experimental feature. I don't understand how that would affect metadata return, but it's possible the thread locking or runnable passing is somehow wrong for the inter-process communication.

Can you reproduce Ed Morley's leaks? That's probably a good place to start debugging if you can't reproduce the crash.
(In reply to Ralph Giles (:rillian) from comment #76)
> In e10s each tab runs in a separate process. This is currently an
> experimental feature. I don't understand how that would affect metadata
> return, but it's possible the thread locking or runnable passing is somehow
> wrong for the inter-process communication.

Same here, I'm not sure how this could affect metadata. Maybe we discovered some kind of new bug?

> Can you reproduce Ed Morley's leaks? That's probably a good place to start
> debugging if you can't reproduce the crash.

Yes, they kind of happen each time I run my tests, but it is a bug already worked on (bug 1022499). Some experimentations I made on these leaks are explained here (https://bugzilla.mozilla.org/show_bug.cgi?id=1016413#c27) and they are pretty misleading. It even seems like the precise amount of bytes leaked depends on the architecture.
Component: Audio/Video → Audio/Video: Playback
Rank: 15
Priority: -- → P2
You need to log in before you can comment on or make changes to this bug.