763010 - expose metadata on media elements

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Ralph Giles (:rillian)]

Most media formats have some kind of tag/metadata support. The bug is about considering exposing this data to web content when a stream is played through a media element.

The decoder libraries we have in gecko generally already parse this information. However, we discard it. There has been no cross-platform interest in adding an interface to query this data to HTML.

We're writing media player applications as part of the WebAPI/B2G effort. While it's possible to load and parse out artist/title/size/duration information from javascript, such applications are also good motivation for exposing the same data from the native parser in gecko.

To resolve we need:

- To pick an API for querying (and setting?) metadata
- To decide if this is the way to go
- A gecko implementation for the formats we support

Comment 1

[Ralph Giles (:rillian)]

Some ideas:

attribute readonly media.tags returns a json object:

{
  'title': 'Sintel',
  'creator': 'Blender Foundation'
  'date': 2011,
  'copyright': '© copyright Blender Foundation | www.sintel.org'
  'licence': 'Creative Commons http://creativecommons.org/licenses/by/3.0/'
}

available after METADATA_LOADED.

Could make it a function media.getMetadata() instead.

Provide tags as is, or map to a standard subset based on dublin code?
Provide XMP as xml or parsed? Generate/merge XMP from native tags?

Handle duplicate keys as a value array?

Add keys for non-tag metadata like bitrate, number of channels, resolution?

How does this integrate with the statistics api proposals?

Comment 2

[David Flanagan [:djf]]

As the author of B2G media apps, this would be a huge help to me.

I think that tags is too short and generic a name.  I'd suggest media.metadata if we're returning an object (a lazily-created object, presumably).  Or media.getMetadata(tag) to return the value of a single named metadata tag.

Returning just the raw tags would be okay, but defining a core subset that were guaranteed to have values (even if they were null) would be nice for documentation purposes.

My immediate need for B2G is for tags like title, artist, album, copyright and so on. The format-type data that I really need is already exposed with duration, videoWidth and videoHeight.  I suppose the format-type metadata would be of interest to some users.  EXIF tags in jpeg images from digital cameras include information like whether the flash was used, and this is of interest to camera geeks.  In the same way, bitrate information would be of interest to music geeks and codec information would be of interest to video geeks.  I can imagine (future) B2G apps that have a mode where they display that kind of information for the power users who really want it.  So in that sense, the more metadata the merrier, I suppose.

Comment 3

[Ralph Giles (:rillian)]

I agree it makes sense to do this for the img tag too.

We'll want to apply CORS taints to the metadata the way we do the pixels, since EXIF data, in particular, can contain sensitive information.

I like returning a lazily created object for simplicity, but that requires some filtering, since there can be things like base64-encoded images in there.

Comment 4

[Ralph Giles (:rillian)]

Raised the idea on http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2012-June/036352.html for feedback.

Ms2ger suggested defining a fixed set of attributes as a separate IDL interface.

Comment 5

[Ralph Giles (:rillian)]

Attachment: work in progress

Work in progress. Chris, does this look like a reasonable direction for how to pass the data up from the codec level?

Returns the first 'artist' and 'title' tags from an Ogg Vorbis file as media.mozCreator and media.mozTitle attributes on the html media elements.

Example test page: https://people.xiph.org/~giles/2012/metadata.html

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 641279 [details] [diff] [review]
work in progress

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

The approach you're using to send metadata up to the media element looks fine.

I'd rather not define a bunch of new moz* fields for metadata. I'd much prefer to make metadata available by returning a JSObject as you suggest, or via a querying API.

There's code in nsFrameMessageManager.cpp to create JSON, maybe that will help you figure out how to JSObjectify the results...

http://mxr.mozilla.org/mozilla-central/search?string=json&find=\.cpp&findi=\.c&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

::: content/html/content/public/nsHTMLMediaElement.h
@@ +43,5 @@
>    typedef mozilla::VideoFrameContainer VideoFrameContainer;
>    typedef mozilla::MediaStream MediaStream;
>    typedef mozilla::MediaResource MediaResource;
>  
> +  typedef nsDataHashtable<nsCStringHashKey, nsCString> MetadataTags;

Maybe these should be nsAString (or perhaps just nsString) so that they can be UTF rather than ASCII? I imagine there will be non ASCII metadata out there in the wild...

::: dom/interfaces/html/nsIDOMHTMLMediaElement.idl
@@ +93,5 @@
>  
> +  // Mozilla extension: return embedded metadata from the stream. This
> +  // can be used by player interfaces to display the song title, artist,
> +  // etc.
> +  readonly attribute DOMString mozCreator; // artist, dc:creator

You're changing the interface so you need to rev the UUID of this class and all subclasses (so audio and video elements).

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Chris Pearce (:cpearce) from comment #6)
> I'd rather not define a bunch of new moz* fields for metadata. I'd much
> prefer to make metadata available by returning a JSObject as you suggest, or
> via a querying API.

Returning a JSObject sounds ideal.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

instead of an attribute it might be better to have a mozGetMetadata() method, since we'll want to create a new object every time.

Comment 9

[Ralph Giles (:rillian)]

Ok, thanks. Ms2ger said something similar about properties being object references rather than copies.

> Maybe these should be nsAString (or perhaps just nsString) so that they can be UTF rather than ASCII? I imagine there will be non ASCII metadata out there in the wild...

khuey said they needed to be concrete nsCString rather than nsACString in the hash table. Can nsCString not carry utf-8? It was my intent to pass the strings in that encoding.

Comment 10

[Maire Reavy [:mreavy]]

Ralph - To help others decide whether this bug is needed for basecamp: Can you summarize why this bug is important to fix, and what it would mean if we shipped b2g without this bug being fixed?  Thanks!

Comment 11

[David Flanagan [:djf]]

I can chime in on that...

The Gaia Gallery and Music apps currently use JavaScript to read metadata from their photo and music files.  They're working okay are are not blocked by this bug.

But the Gaia Video app also needs to read metadata to determine, at a minimum, the title of a video, if that information is available.

The Video app could do this in JavaScript, but I gather that video metadata is more complicated (less standardized, and more formats) than photo and music metadata. I've been told that Gecko already reads the metadata and it is just a matter of exposing it.  If so, it is simpler to do it this way than to rewrite the metadata parsing code in JavaScript.

I don't see this as a super high-priority bug.  It is blocking one feature of one Gaia app.  It would also potentially give us a big performance win to be able to get photo and music metadata from gecko.  We don't currently have bugs filed on the performance of the Gallery and Music apps on large media collections, but faster is always better.

Comment 12

[Ralph Giles (:rillian)]

Thanks, David.

So, if we shipped b2g without this fixed, performance indexing media files would be worse, and substitute parsers may have to be written in js.

Comment 13

[Timothy B. Terriberry (:derf)]

This looks like it's using the loadedmetadata event to signal that these values are available. AFAIK, that's only fired when the readyState changes from HAVE_NOTHING to HAVE_METADATA. Has anyone thought through how we would support chained files with this API, which can update their metadata after reaching the HAVE_CURRENT_DATA or later readyStates? Presumably the webpage will need some notification that there's been an update.

Comment 14

[Ralph Giles (:rillian)]

The way e.g. the TextTracks spec handles this is to have the attribute return a live object, which has an onchanged callback member.

If we went with Roc's suggestion of a method which returns a new object every time, we'd need to define an onmetadatachanged event callback on the media element instead.

Are you suggesting we add that signal now, rather than later? That would help inform the API choice. Until Friday's priority discussion I was going with 'whatever I can get done in time'.

There's some discussion on this dev.webapi thread: https://groups.google.com/forum/?fromgroups#!topic/mozilla.dev.webapi/b46F9nt6wdA

Comment 15

[Ralph Giles (:rillian)]

Attachment: work in progress

Updated patch. This exposes creator and title from Ogg Vorbis files through a jsobject created by the media.mozMetadata attribute.

Comment 16

[Dietrich Ayala (:dietrich)]

Blocking- for V1, as there's a Gaia workaround.

Comment 17

[Ralph Giles (:rillian)]

Attachment: work in progress

Updated patch. This works for Ogg Vorbis files, but I think it leaks the strings.

Attribute replaced with a media.mozGetMetadata() method, to advertise that one gets a separate copy each time, based on feedback.

Returns the raw tag names without filtering or normalization. Be sure to call .toLowerCase on the tag names if you want to match them with anything.

Comment 18

[Ralph Giles (:rillian)]

Attachment: add media.mozGetMetadata method

Ready for review.

Comment 19

[Ralph Giles (:rillian)]

Attachment: add mochitest for media.mozGetMetadata()

Comment 20

[Ralph Giles (:rillian)]

Attachment: add mochitest for media.mozGetMetadata()

Update to the mochitest patch. Call monitor.finished() instead of SimpleTest.finish() to avoid confusing the test harness about subsequent tests, and add another test file to the list.

Comment 21

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Dietrich Ayala (:dietrich) from comment #16)
> Blocking- for V1, as there's a Gaia workaround.

https://github.com/mozilla-b2g/gaia/issues/2277

The workaround has it's own pref issue, so if we are shipping the workaround then that need to be fixed (marking blocking-basecamp? on Github)

Comment 22

[Ralph Giles (:rillian)]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TW) from comment #21)

> The workaround has it's own pref issue, so if we are shipping the workaround
> then that need to be fixed (marking blocking-basecamp? on Github)

To be clear, the current patch only works for Ogg Vorbis files using nsBuiltinDecoder. Based on David's response in comment #11 and Dietrich's Blocking-, I'm not planning to extend the patch to support mp3, etc. with the B2G stagefright decoders in time for basecamp. Instead I'll be working on Opus support for WebRTC for the next few weeks.

Comment 23

[Mozilla RelEng Bot]

Try run for 959b45fd2c08 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=959b45fd2c08
Results (out of 17 total builds):
    exception: 7
    failure: 10
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-959b45fd2c08

Comment 24

[Ralph Giles (:rillian)]

(In reply to Mozilla RelEng Bot from comment #23)

>     https://tbpl.mozilla.org/?tree=Try&rev=959b45fd2c08
> Results (out of 17 total builds):
>     exception: 7
>     failure: 10

I cancelled this to rerun with only the plain mochitests. The results aren't meaningful.

Most recent patches testing at https://tbpl.mozilla.org/?tree=Try&rev=0832a383bee6

> Builds (or logs if builds failed) available at:
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-
> 959b45fd2c08

Comment 25

[Ralph Giles (:rillian)]

tbpl bot being confusing is bug 674681.

Comment 26

[Mozilla RelEng Bot]

Try run for 959b45fd2c08 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=959b45fd2c08
Results (out of 17 total builds):
    exception: 7
    failure: 10
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-959b45fd2c08

Comment 27

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 645126 [details] [diff] [review]
add media.mozGetMetadata method

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

Looks pretty good. Mostly style nits. r+ with the following changes.

::: content/html/content/public/nsHTMLMediaElement.h
@@ +707,5 @@
> +  // and convert it to a JSObject.
> +  static PLDHashOperator BuildObjectFromTags(nsCStringHashKey::KeyType aKey,
> +                                             nsCString aValue,
> +                                             void* aUserArg);
> +  nsAutoPtr<MetadataTags> mTags;

I think the metadata tags should be read-only to HTMLMediaElement. So can this be |nsAutoPtr<const MetadataTags> mTags;|?

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +1438,5 @@
> +{
> +  MetadataIterCx* args = static_cast<MetadataIterCx*>(aUserArg);
> +
> +  nsString wideValue = NS_ConvertUTF8toUTF16(aValue);
> +  JSString *string = JS_NewUCStringCopyZ(args->cx, wideValue.Data());

Our convention for * in pointers is "Type* varName". So |string| should be declared as |JSString* string|.

@@ +1448,5 @@
> +  return PL_DHASH_NEXT;
> +}
> +
> +NS_IMETHODIMP
> +nsHTMLMediaElement::MozGetMetadata(JSContext *cx, JS::Value* aValue)

(JSContext* aCx, JS::Value* aValue)

@@ +1666,5 @@
>      mLoadWaitStatus(NOT_WAITING),
>      mVolume(1.0),
>      mChannels(0),
>      mRate(0),
> +    mTags(nsnull),

nsAutoPtr should null this for you already.

@@ +2676,5 @@
>    if (start > 0.0) {
>      SetCurrentTime(start);
>      mFragmentStart = start;
>    }
>  }

You should also reset mTags in AbortExistingLoads(), otherwise when a new load happens, the metadata will still be present in the media element. In particular if the load fails and we never receive a loadedmetadata event with new/null MetadataTags.

@@ +2683,5 @@
>  {
>    mChannels = aChannels;
>    mRate = aRate;
>    mHasAudio = aHasAudio;
> +  if (mTags) {

mTags is an nsAutoPtr, so you can just set mTags=aTags and mTags will delete its old contents, if any.

::: content/media/nsBuiltinDecoder.cpp
@@ +406,5 @@
>  
>  void nsBuiltinDecoder::MetadataLoaded(PRUint32 aChannels,
>                                        PRUint32 aRate,
> +                                      bool aHasAudio,
> +                                      nsHTMLMediaElement::MetadataTags* aTags)

Can aTags be const?

::: content/media/nsBuiltinDecoderReader.h
@@ +51,5 @@
>  
>    // True if we have an active video bitstream.
>    bool mHasVideo;
> +
> +  // Tag metadata from the media stream

Full stop at the end of sentence please.

@@ +52,5 @@
>    // True if we have an active video bitstream.
>    bool mHasVideo;
> +
> +  // Tag metadata from the media stream
> +  nsHTMLMediaElement::MetadataTags* mTags;

I'm a bit concerned about the life cycle of mTags; it's allocated on the decode thread, and a non-owning reference to it is stored inside the decoder's nsVideoInfo, but the owning reference is held by the main-thread-only media element. At any time the media element could destroy mTags, and the decoder would have a dangling pointer (modulo careful management, etc...).

I'd like to avoid (any more!) situations where careful destruction order of our objects is required, and I don't think we'll need to access the metadata in the decoder (right?), so I think we should instead pass in a MetadataTags** to nsBuiltinDecoderReader::ReadMetadata(). Then we don't need the reference to MetadataTags in nsVideoInfo, and we can send the only reference to the MetadataTags* over to the media element via the metadataloaded event as you're doing now, and it will be impossible for us to have any dangling pointers.

You can just ignore the MetadataTags parameter to ReadMetadata() for the other reader subclasses for now. Please file follow up bugs to implement metadata reading in the other backends.

::: content/media/nsBuiltinDecoderStateMachine.cpp
@@ +121,5 @@
>    nsCOMPtr<nsBuiltinDecoder> mDecoder;
>  public:
>    nsAudioMetadataEventRunner(nsBuiltinDecoder* aDecoder, PRUint32 aChannels,
> +                             PRUint32 aRate, bool aHasAudio,
> +                             nsHTMLMediaElement::MetadataTags *aTags) :

nsHTMLMediaElement::MetadataTags* aTags

@@ +139,5 @@
>  
>    const PRUint32 mChannels;
>    const PRUint32 mRate;
>    const bool mHasAudio;
> +  nsHTMLMediaElement::MetadataTags* mTags;

This can be |const nsHTMLMediaElement::MetadataTags*| right?

@@ +1802,5 @@
>      PRUint32 frameBufferLength = mInfo.mAudioChannels * FRAMEBUFFER_LENGTH_PER_CHANNEL;
>      mDecoder->RequestFrameBufferLength(frameBufferLength);
>    }
>    nsCOMPtr<nsIRunnable> metadataLoadedEvent =
> +    new nsAudioMetadataEventRunner(mDecoder, mInfo.mAudioChannels, mInfo.mAudioRate, HasAudio(), mInfo.mTags);

For a long line like this, we'd indent like so:

    new nsAudioMetadataEventRunner(mDecoder,
                                   mInfo.mAudioChannels,
                                   mInfo.mAudioRate,
                                   HasAudio(),
                                   mInfo.mTags);

(That only looks correctly indented in a monospace font of course...)

The definition of long line is roughly "longer than the longest line existing in the file". ;)

::: content/media/ogg/nsOggReader.cpp
@@ +156,5 @@
> +{
> +  nsHTMLMediaElement::MetadataTags* tags;
> +  int i;
> +
> +  tags = new nsHTMLMediaElement::MetadataTags;

Our new operator is infallible now, so you don't need to null check this.

Additionally, if we were to null check this, our existing convention is to exit early so we can avoid the indentation in the following block.

@@ +159,5 @@
> +
> +  tags = new nsHTMLMediaElement::MetadataTags;
> +  if (tags) {
> +    tags->Init();
> +    for (i = 0; i < vc->comments; i++) {

Declare i here to reduce its scope and live time, i.e. "for (int i...".

@@ +166,5 @@
> +      if (!div) {
> +        LOG(PR_LOG_DEBUG, ("Invalid vorbis comment: no separator"));
> +        continue;
> +      }
> +      // this should be ASCII

Capital at start of and full stop at end of sentences please. Ditto below.

Comment 28

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 645126 [details] [diff] [review]
add media.mozGetMetadata method

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

Looks pretty good. Mostly style nits. r+ with the following changes.

::: content/html/content/public/nsHTMLMediaElement.h
@@ +707,5 @@
> +  // and convert it to a JSObject.
> +  static PLDHashOperator BuildObjectFromTags(nsCStringHashKey::KeyType aKey,
> +                                             nsCString aValue,
> +                                             void* aUserArg);
> +  nsAutoPtr<MetadataTags> mTags;

I think the metadata tags should be read-only to HTMLMediaElement. So can this be |nsAutoPtr<const MetadataTags> mTags;|?

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +1438,5 @@
> +{
> +  MetadataIterCx* args = static_cast<MetadataIterCx*>(aUserArg);
> +
> +  nsString wideValue = NS_ConvertUTF8toUTF16(aValue);
> +  JSString *string = JS_NewUCStringCopyZ(args->cx, wideValue.Data());

Our convention for * in pointers is "Type* varName". So |string| should be declared as |JSString* string|.

@@ +1448,5 @@
> +  return PL_DHASH_NEXT;
> +}
> +
> +NS_IMETHODIMP
> +nsHTMLMediaElement::MozGetMetadata(JSContext *cx, JS::Value* aValue)

(JSContext* aCx, JS::Value* aValue)

@@ +1451,5 @@
> +NS_IMETHODIMP
> +nsHTMLMediaElement::MozGetMetadata(JSContext *cx, JS::Value* aValue)
> +{
> +  if (!mTags) {
> +    return NS_ERROR_DOM_INVALID_STATE_ERR;

Wait, shouldn't you just return an empty object instead?

I think it would be OK to throw DOM_INVALID_STATE_ERR if readyState < HAVE_METADATA, but if we have metadata, we should return an empty object, rather than requiring the caller to always wrap their mozGetMetadata call in a try/catch block.

@@ +1666,5 @@
>      mLoadWaitStatus(NOT_WAITING),
>      mVolume(1.0),
>      mChannels(0),
>      mRate(0),
> +    mTags(nsnull),

nsAutoPtr should null this for you already.

@@ +2676,5 @@
>    if (start > 0.0) {
>      SetCurrentTime(start);
>      mFragmentStart = start;
>    }
>  }

You should also reset mTags in AbortExistingLoads(), otherwise when a new load happens, the metadata will still be present in the media element. In particular if the load fails and we never receive a loadedmetadata event with new/null MetadataTags.

@@ +2683,5 @@
>  {
>    mChannels = aChannels;
>    mRate = aRate;
>    mHasAudio = aHasAudio;
> +  if (mTags) {

mTags is an nsAutoPtr, so you can just set mTags=aTags and mTags will delete its old contents, if any.

::: content/media/nsBuiltinDecoder.cpp
@@ +406,5 @@
>  
>  void nsBuiltinDecoder::MetadataLoaded(PRUint32 aChannels,
>                                        PRUint32 aRate,
> +                                      bool aHasAudio,
> +                                      nsHTMLMediaElement::MetadataTags* aTags)

Can aTags be const?

::: content/media/nsBuiltinDecoderReader.h
@@ +51,5 @@
>  
>    // True if we have an active video bitstream.
>    bool mHasVideo;
> +
> +  // Tag metadata from the media stream

Full stop at the end of sentence please.

@@ +52,5 @@
>    // True if we have an active video bitstream.
>    bool mHasVideo;
> +
> +  // Tag metadata from the media stream
> +  nsHTMLMediaElement::MetadataTags* mTags;

I'm a bit concerned about the life cycle of mTags; it's allocated on the decode thread, and a non-owning reference to it is stored inside the decoder's nsVideoInfo, but the owning reference is held by the main-thread-only media element. At any time the media element could destroy mTags, and the decoder would have a dangling pointer (modulo careful management, etc...).

I'd like to avoid (any more!) situations where careful destruction order of our objects is required, and I don't think we'll need to access the metadata in the decoder (right?), so I think we should instead pass in a MetadataTags** to nsBuiltinDecoderReader::ReadMetadata(). Then we don't need the reference to MetadataTags in nsVideoInfo, and we can send the only reference to the MetadataTags* over to the media element via the metadataloaded event as you're doing now, and it will be impossible for us to have any dangling pointers.

You can just ignore the MetadataTags parameter to ReadMetadata() for the other reader subclasses for now. Please file follow up bugs to implement metadata reading in the other backends.

::: content/media/nsBuiltinDecoderStateMachine.cpp
@@ +121,5 @@
>    nsCOMPtr<nsBuiltinDecoder> mDecoder;
>  public:
>    nsAudioMetadataEventRunner(nsBuiltinDecoder* aDecoder, PRUint32 aChannels,
> +                             PRUint32 aRate, bool aHasAudio,
> +                             nsHTMLMediaElement::MetadataTags *aTags) :

nsHTMLMediaElement::MetadataTags* aTags

@@ +139,5 @@
>  
>    const PRUint32 mChannels;
>    const PRUint32 mRate;
>    const bool mHasAudio;
> +  nsHTMLMediaElement::MetadataTags* mTags;

This can be |const nsHTMLMediaElement::MetadataTags*| right?

@@ +1802,5 @@
>      PRUint32 frameBufferLength = mInfo.mAudioChannels * FRAMEBUFFER_LENGTH_PER_CHANNEL;
>      mDecoder->RequestFrameBufferLength(frameBufferLength);
>    }
>    nsCOMPtr<nsIRunnable> metadataLoadedEvent =
> +    new nsAudioMetadataEventRunner(mDecoder, mInfo.mAudioChannels, mInfo.mAudioRate, HasAudio(), mInfo.mTags);

For a long line like this, we'd indent like so:

    new nsAudioMetadataEventRunner(mDecoder,
                                   mInfo.mAudioChannels,
                                   mInfo.mAudioRate,
                                   HasAudio(),
                                   mInfo.mTags);

(That only looks correctly indented in a monospace font of course...)

The definition of long line is roughly "longer than the longest line existing in the file". ;)

::: content/media/ogg/nsOggReader.cpp
@@ +156,5 @@
> +{
> +  nsHTMLMediaElement::MetadataTags* tags;
> +  int i;
> +
> +  tags = new nsHTMLMediaElement::MetadataTags;

Our new operator is infallible now, so you don't need to null check this.

Additionally, if we were to null check this, our existing convention is to exit early so we can avoid the indentation in the following block.

@@ +159,5 @@
> +
> +  tags = new nsHTMLMediaElement::MetadataTags;
> +  if (tags) {
> +    tags->Init();
> +    for (i = 0; i < vc->comments; i++) {

Declare i here to reduce its scope and live time, i.e. "for (int i...".

@@ +166,5 @@
> +      if (!div) {
> +        LOG(PR_LOG_DEBUG, ("Invalid vorbis comment: no separator"));
> +        continue;
> +      }
> +      // this should be ASCII

Capital at start of and full stop at end of sentences please. Ditto below.

::: dom/interfaces/html/nsIDOMHTMLMediaElement.idl
@@ +94,5 @@
> +  // Mozilla extension: return embedded metadata from the stream as a
> +  // JSObject with key:value pairs for each tag. This can be used by
> +  // player interfaces to display the song title, artist, etc.
> +  [implicit_jscontext]
> +  jsval mozGetMetadata();

How about making this a "readonly attribute jsval mozMetadata" ?

Comment 29

[Chris Pearce [:cpearce (Not reading bugmail)]]

Oh lame. I added additional comments and it reproduced all my original comments again. Here's the additional comments I added:


@@ +1451,5 @@
> +NS_IMETHODIMP
> +nsHTMLMediaElement::MozGetMetadata(JSContext *cx, JS::Value* aValue)
> +{
> +  if (!mTags) {
> +    return NS_ERROR_DOM_INVALID_STATE_ERR;

Wait, shouldn't you just return an empty object instead?

I think it would be OK to throw DOM_INVALID_STATE_ERR if readyState < HAVE_METADATA, but if we have metadata, we should return an empty object, rather than requiring the caller to always wrap their mozGetMetadata call in a try/catch block.

::: dom/interfaces/html/nsIDOMHTMLMediaElement.idl
@@ +94,5 @@
> +  // Mozilla extension: return embedded metadata from the stream as a
> +  // JSObject with key:value pairs for each tag. This can be used by
> +  // player interfaces to display the song title, artist, etc.
> +  [implicit_jscontext]
> +  jsval mozGetMetadata();

How about making this a "readonly attribute jsval mozMetadata" ?

Comment 30

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 645484 [details] [diff] [review]
add mochitest for media.mozGetMetadata()

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

Nice.

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

I think we should have some files with no metadata so that we test that path.

Comment 31

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Chris Pearce (:cpearce) from comment #29)
> ::: dom/interfaces/html/nsIDOMHTMLMediaElement.idl
> @@ +94,5 @@
> > +  // Mozilla extension: return embedded metadata from the stream as a
> > +  // JSObject with key:value pairs for each tag. This can be used by
> > +  // player interfaces to display the song title, artist, etc.
> > +  [implicit_jscontext]
> > +  jsval mozGetMetadata();
> 
> How about making this a "readonly attribute jsval mozMetadata" ?

As discussed to death earlier (but probably not in the bug), keep the method.

Comment 32

[Boris Zbarsky [:bzbarsky]]

To expand on that, WebIDL generally makes it hard to have attributes whose getter returns a different thing each time, with good reason: it's very confusing to consumers if this code:

  foo.bar.x = 5;
  alert(foo.bar.x == 5);

alerts false.

Comment 33

[Ralph Giles (:rillian)]

(In reply to Boris Zbarsky (:bz) from comment #32)

>   foo.bar.x = 5;
>   alert(foo.bar.x == 5);
> 
> alerts false.

I'm sorry, I don't understand your example here. Isn't this a getter not returning the same object handed in by a previous setter? The proposal is for a read-only attribute.

Also, don't numbers compare by value rather than by reference? E.g. did you mean:

  media.mozMetadata = y;
  alert(media.mozMetadata === y); // alerts false!

?

Comment 34

[Boris Zbarsky [:bzbarsky]]

In comment 32, the getter is for the foo.bar get.  The .x is an expando property the page then sets on the resulting object for its own purposes.

In terms of your case this would be:

  media.mozMetadata.somethingIWantToAdd = 5;
  alert(media.mozMetadata.somethingIWantToAdd == 5);

Comment 35

[Ralph Giles (:rillian)]

(In reply to Boris Zbarsky (:bz) from comment #34)
> In comment 32, the getter is for the foo.bar get.  The .x is an expando

Thanks for explaining. I see what you mean now. Can you also address why this is not resolved by making the property with the getter readonly?

Comment 36

[Ralph Giles (:rillian)]

(In reply to :Ms2ger from comment #31)

> As discussed to death earlier (but probably not in the bug), keep the method.

Not everone

Comment 37

[Ralph Giles (:rillian)]

*sigh*

(In reply to :Ms2ger from comment #31)

> As discussed to death earlier (but probably not in the bug), keep the method.

Not everyone followed the discussions, so we'd appreciate it if you could clarify your reasoning. I understood from Jonas' nice explanation on webapi-dev (https://groups.google.com/forum/?fromgroups#!searchin/mozilla.dev.webapi/metadata/mozilla.dev.webapi/b46F9nt6wdA/overview) that there's a style preference for properties to always return the same object (a weak one in my informal surveys) my concern is the following:

Lots of new HTML APIs are making use of 'live' objects, and in particular I've been looking at the media.*Tracks properties which return objects (with array-like getters) which are supposed to be dynamically update.

That's not what this patch does, but it feels like we're making this api more ugly than it needs to be just to delay implementing something we'll do later anyway. Live objects also have the advantage that, when we implement metadata update during resource playback, we can hang the onchanged callback on the mozMetadata object itself, instead of having to add a new event at the media element layer.

Comment 38

[Ralph Giles (:rillian)]

(In reply to Ralph Giles (:rillian) from comment #35)
> Can you also address why
> this is not resolved by making the property with the getter readonly?

:cpearce on irc said this is because a readonly attribute affects only the object reference in that slot; the object itself might be mutable. And I'm trying to return a open-ended dictionary of everything returned by the media decoder here, so defining and iterface with readonly attributes inside the mozMetadata object doesn't work.

Comment 39

[Chris Pearce [:cpearce (Not reading bugmail)]]

So I don't understand why we shouldn't construct a JS object and store a reference to that in HTMLMediaElement and then just return that reference whenever (readonly attribute) HTMLMediaElment.mozMetadata is read? Ralph tells me that ms2ger told him that he shouldn't do that for some reason, but didn't say why. I'd like to understand why.

Comment 40

[Boris Zbarsky [:bzbarsky]]

OK.  Hold on.  Back up.

There are two things here.  One is how we want to do this once it's finally working right (with WebIDL and all that).  The other is what we do for now.  There's also the obvious question of how what we do for now affects our ability to do the right thing later.  I guess we can count that as a third thing.

It sounds like the eventual end goal is to return an object here which represents some sort of live (and liable to change) set of name->value mappings, right?  In WebIDL terms that means returning an instance of an interface with a "getter" that takes a string.

Is that understanding correct?  If not, what _is_ the eventual goal, assuming we could magically return exactly the sort of thing we want to return?

In terms of comment 39, the proposal as stated has the problem that one consumer could change what metadata others see...  If the properties on the JS object were marked readonly, that would be a big step up; at that point the only obvious problem apart from probably not being what we want long-term is that we'd need to handle all the GC tracing bits.

Comment 41

[Boris Zbarsky [:bzbarsky]]

And one more question.  What are our timeframes here, both in terms of getting "something" checked in and in terms of getting to the correct final state?

Comment 42

[Ralph Giles (:rillian)]

(In reply to Boris Zbarsky (:bz) from comment #40)

> It sounds like the eventual end goal is to return an object here which
> represents some sort of live (and liable to change) set of name->value
> mappings, right?

That was my goal, yes.

WebIDL has a dictionary type, which is the kind of thing I actually want to return. They're represented in js by objects, but passed by value and, "any dictionary returned from a platform object will be a copy and modifications made to it will not be visible to the platform object."[1] Which sounds like what the current patch does, down to writable properties on the constructed object copy.

However, it looks like WebIDL dictionary member names have to be ASCII identifiers, which is more restrictive than the metadata formats we want to return.

> In WebIDL terms that means returning an instance of an
> interface with a "getter" that takes a string.

Ok. Does that string getter look like a normal javascript property getter/setter, so one could still do media.mozMetadata['dc:creator'] or media.mozMetadata.title? Or are we talking about media.mozMetadata.GetTag('dc:creator')?

> In terms of comment 39, the proposal as stated has the problem that one
> consumer could change what metadata others see...

Why is that a problem? If callers can add expando properties, why shouldn't be able to mutate the properties set by the decoder? For example, one might want to the object through a normalizer whenever the metadata updates, and have those keys/values reflected later in other contexts which hold a reference to that object.

> If the properties on the JS object were marked readonly, that would be a big step up;

It looks straightforward to do this with the jsapi when we create the properties.

> at that point
> the only obvious problem apart from probably not being what we want
> long-term is that we'd need to handle all the GC tracing bits.

Ok. So telling the js engine gc we have a reference to an object in nsHTMLMediaElement is new code? My other goal here is to understand how to implement live objects for the TrackList interface. (I think these are platform array objects in WebIDL.)


[1] http://dev.w3.org/2006/webapi/WebIDL/#idl-dictionaries

Comment 43

[Ralph Giles (:rillian)]

(In reply to Boris Zbarsky (:bz) from comment #41)
> And one more question.  What are our timeframes here, both in terms of
> getting "something" checked in and in terms of getting to the correct final
> state?

Very good question. When I thought this was on the critical path for b2g, I was going for 'whatever I can get working first'. Now it seems we have more time to think about the design, but I have webrtc work I want to concentrate on for the next few weeks. I was hoping to land this as the basis for new work and get it off my plate.

Comment 44

[Boris Zbarsky [:bzbarsky]]

> WebIDL has a dictionary type,

When converting a dictionary to a JS value, a new JS object is always created.  So using a dictionary type wouldn't do what you say the goal is.  Certainly not if the property values need to change later on for an object page script already has a reference to.

> Does that string getter look like a normal javascript property getter/setter

Yes, exactly.

> Why is that a problem?

Well, not least because it would mean browser chrome has no way to get the correct metadata, right?

> If callers can add expando properties

Expando properties are suppressed in various contexts, including the browser chrome case.

> For example, one might want to the object through a normalizer whenever the metadata
> updates

I'm not sure there's a sane way to have an object that is written to by both the page _and_ the underlying implementation without all sorts of issues due to races and such.  Conceptually.  Obviously, we _could_ implement such a thing; I just don't think we should.

> It looks straightforward to do this with the jsapi when we create the properties.

Indeed.

> So telling the js engine gc we have a reference to an object in nsHTMLMediaElement is
> new code?

Yes.

> I think these are platform array objects in WebIDL.

We need to come up with an implementation plan for those, yes.  The WebIDL spec itself has a lot of issues around platform array objects, sadly... :(  If this is blocking work, please let me know and I'll raise the priority of sorting that stuff out.

As far as comment 43, it sounds like it should be ok to land something for now as long as there is a clear plan to getting it to the desired end state.

Comment 45

[Ralph Giles (:rillian)]

Attachment: add media.mozGetMetadata method

Updated patch in response to review comments. Chris, please check this new version, especially the bit in AbortExistingLoads() were I also reset mChannels and mRate.

 - Fixed style nits and formatting.
 - const-ified mTags.
 - Removed redundant null checks.
 - Removed nsVideoInfo::mTags in favour of passing a pointer to ReadMetadata().
 - Throw an exception when mozGetMetadata is called before HAVE_METADATA or if it can't create a JSObject.
 - Otherwise, return an empty JSObject if nsHTMLMediaElement::mTags is somehow null.

Comment 46

[Ralph Giles (:rillian)]

Attachment: add mochitest for media.mozGetMetadata()

Updated patch in response to comments:

 - Add sounds.ogg which has no comments to the metadata test to verify {} return
 - Test exception throw on early mozGetMetadata() calls.

Comment 47

[Ralph Giles (:rillian)]

Thanks, Chris. The latest patches show no new issues on try: https://tbpl.mozilla.org/?tree=Try&rev=ef0efa936863

Comment 48

[Ralph Giles (:rillian)]

(In reply to Boris Zbarsky (:bz) from comment #44)

> > Does that string getter look like a normal javascript property getter/setter
> 
> Yes, exactly.

Ok, good.

> Well, not least because it would mean browser chrome has no way to get the
> correct metadata, right?

Not a use case I'd considered. This is for web-app media players, remember. Still, doesn't bode well for the statistics api...

> > So telling the js engine gc we have a reference to an object in nsHTMLMediaElement is
> > new code?
> 
> Yes.

Fascinating.

> We need to come up with an implementation plan for those, yes.  The WebIDL
> spec itself has a lot of issues around platform array objects, sadly... :( 
> If this is blocking work, please let me know and I'll raise the priority of
> sorting that stuff out.

I'm not currently blocked on that. I'm hoping to look at it in a few months when I get back to the text track stuff. padenot has also been looking at taking the TrackList implementation on.

> As far as comment 43, it sounds like it should be ok to land something for
> now as long as there is a clear plan to getting it to the desired end state.

I think Boris has made convincing arguments why the live object idea isn't as attractive as it seems. I propose to land the patch, as is, if no one objects further before next week.

That means:

- A media.mozGetMetadata() method which returns a new object copy each time.
- When we add support for metadata updates during playback, we'll need to add a new 'changedmetadata' event on the media element for notification.

I'm really enjoying learning about these aspects gecko, so thanks for taking time to explain, bz.

Comment 49

[Boris Zbarsky [:bzbarsky]]

> Fascinating.

Well, it's not new _infrastructure_, to be clear.  We have ways of communicating that info.  You'd just need to add the right boilerplate stuff to mediaelement (some cycle collection goop and so forth).  It's more code than one would want, but not as much as it would take to implement it all from scratch.  ;)

In a few months I'd hope we have platform array objects figured out.

The landing plan sounds good to me and should be pretty straightforward to do, I think.

Comment 50

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6afcf737da50
https://hg.mozilla.org/integration/mozilla-inbound/rev/276d0abab1ab

Comment 51

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6afcf737da50
https://hg.mozilla.org/mozilla-central/rev/276d0abab1ab

Comment 52

[Ralph Giles (:rillian)]

I've added basic documentation to https://developer.mozilla.org/en-US/docs/DOM/HTMLMediaElement

Comment 53

[Jean-Yves Perrier [:teoli]]

Thank you very much!

I've added a line in https://developer.mozilla.org/en-US/docs/Firefox_17_for_developers too
(Ralph: once documented, we flip dev-doc-needed to dev-doc-complete, so that we can track what has been done.)

Comment 54

[David Flanagan [:djf]]

Don't we only support metadata for Ogg files (video, audio or both?) right now?  Should the docs mention that?  Currently they give the impression that it works for any media type.

Comment 55

[Ralph Giles (:rillian)]

(In reply to Jean-Yves Perrier [:teoli] from comment #53)

> (Ralph: once documented, we flip dev-doc-needed to dev-doc-complete, so that
> we can track what has been done.)

Aha. Thanks for the pointer!

(In reply to David Flanagan [:djf] from comment #54)
> Don't we only support metadata for Ogg files (video, audio or both?) right
> now?  Should the docs mention that?  Currently they give the impression that
> it works for any media type.

Firefox 17 only returns metadata from the vorbis streams in Ogg files (so audio only, but you might get something for video, depending on how the tracks are tagged). There are open bugs about supporting other formats. Some should land for firefox 18, but not all.