778050 - Support Opus tag metadata

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Ralph Giles (:rillian)]

Opus files have key=value metadata in the second header packet, in a similar format to vorbis and theora. Currently we ignore this.

Instead we should parse this header and make the data available through media.mozGetMetadata(). libopus doesn't container a parser, so we'd need to write one and use it fill in an nsHTMLMediaElement::MetadataTags hashtable, to be returned by nsOggReader::ReadMetadata() as it is for vorbis.

Comment 1

[Ralph Giles (:rillian)]

Attachment: add Opus support to mozGetMetadata

Can test with https://people.xiph.org/~giles/2012/metadata-opus.html

The parse code is a bit hybrid. 

I copy the raw comment strings into an array of nsCString in ::DecodeHeader, and only do full validation when generating the hash table. This makes us no more strict than before.

I convert the comments back to C strings and parse them the same was I did for Vorbis, which seemed shorter, but it's a bit of a hybrid between C strings and nsCStrings. I considered parsing them into an array of char* instead, if you like that better. Parsing them into a vorbis_comment struct would let us share the validation and hash table generation, but introduces a dependency between the two codecs.

Comment 2

[Ralph Giles (:rillian)]

Attachment: Support Opus tag metadata

an array member variable. Add a method to generate
the nsHTMLMediaElement::MetadataTags hash table from
those entries, performing the same validatation we
do for Vorbis.

IsValidVorbisTagName is promoted to a static method
on nsOggReader so we can use it externally.

This feature is tested by adding the existing opus
test file to gMetadataTests.

Comment 3

[Ralph Giles (:rillian)]

Comment on attachment 660608 [details] [diff] [review]
Support Opus tag metadata

Update patch to include a test case.

Comment 4

[Ralph Giles (:rillian)]

Attachment: Support Opus tag metadata

Update to fix a debug message typo. Sorry for the noise.

pushed to try as https://tbpl.mozilla.org/?tree=Try&rev=803d96343a8b

Comment 5

[Ralph Giles (:rillian)]

Try push is green. Orange is three timeouts in unrelated test sections.

Comment 6

[Timothy B. Terriberry (:derf)]

Comment on attachment 660613 [details] [diff] [review]
Support Opus tag metadata

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

I don't think there's anything functionally wrong with this patch, so I'll r+ it, but I think we could organize things a little better. Feel free to rework and resubmit if you agree.

::: content/media/ogg/nsOggCodecState.cpp
@@ +930,1 @@
>        uint32_t bytes = aPacket->bytes - 8;

There's a comment at line 922 here that says things that are untrue after applying this patch. Please update it.

@@ +934,5 @@
>        buf += 4;
>        bytes -= 4;
>        if (len > bytes)
>          return false;
> +      mVendorString = nsCString(reinterpret_cast<const char*>(buf), len);

I'd personally prefer static_cast<> here (and where used below), though I can't imagine it ever making a practical difference.

@@ +983,5 @@
>    }
>    return true;
>  }
>  
> +/* Construct and return a tags hashmap from our internal array */

Apart from the codec duplication (see below), the thing that bothers me here is that this lives in a completely different file from TagsFromVorbisComment(). If they were in the same place, then if someone updates one, there's at least a fighting chance they'll notice and update the other. Perhaps put this function in nsOggReader and have it take a const nsTArray<nsCstring> & returned by nsOpusState::GetTags()? That would also avoid the need to include nsOggReader.h here.

Alternatively, you could make this a virtual method of nsOggCodecState, with a default implementation that just returns NULL until we implement tag parsing for all codecs, and have both implementations in nsOggCodecState. That approach feels somewhat cleaner to me.

@@ +992,5 @@
> +  tags = new nsHTMLMediaElement::MetadataTags;
> +  tags->Init();
> +  for (uint32_t i = 0; i < mTags.Length(); i++) {
> +    const char* comment = mTags[i].Data();
> +    uint32_t length = mTags[i].Length();

Everything from here down to the Put() call is identical to the code in TagsFromVorbisComment(), except for the debug logging. I'd recommend putting this in a
  static void AddVorbisComment(nsHtmlMediaElement *aTags, const char *aComment, uint32_t aLength)
function that does all three checks (and the Put(), too).

::: content/media/ogg/nsOggReader.cpp
@@ +159,5 @@
>      }
>    }
>  }
>  
> +bool nsOggReader::IsValidVorbisTagName(nsCString& name)

I know this isn't new code, but while you're here, isn't local style to call this aName?

Comment 7

[Ralph Giles (:rillian)]

Comment on attachment 660613 [details] [diff] [review]
Support Opus tag metadata

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

Ok, thanks for the comments. I'll see if I can move the calls to the same file in a sensible way.

::: content/media/ogg/nsOggCodecState.cpp
@@ +930,1 @@
>        uint32_t bytes = aPacket->bytes - 8;

Thanks.

@@ +934,5 @@
>        buf += 4;
>        bytes -= 4;
>        if (len > bytes)
>          return false;
> +      mVendorString = nsCString(reinterpret_cast<const char*>(buf), len);

static_cast doesn't work here. It will only perform a cast that could be done by an implicit conversion, and C++ won't implicitly convert unsigned to signed.

C-style casts work of course, but cpearce has asked for C++ casts as part of local style in the past.

::: content/media/ogg/nsOggReader.cpp
@@ +159,5 @@
>      }
>    }
>  }
>  
> +bool nsOggReader::IsValidVorbisTagName(nsCString& name)

Good catch.

Comment 8

[Ralph Giles (:rillian)]

Attachment: updated fix

Updating in response to comments. I moved things to nsOggCodecState instead of into nsOggReader. That seemed to make more sense to me.

Comment 9

[Ralph Giles (:rillian)]

Pushed to try as https://tbpl.mozilla.org/?tree=Try&rev=ef0751b8a559

Comment 10

[Timothy B. Terriberry (:derf)]

Comment on attachment 661439 [details] [diff] [review]
updated fix

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

This looks much cleaner, thanks!

::: content/media/ogg/nsOggCodecState.cpp
@@ +122,5 @@
> +  // excluding 0x3D '=' which is the separator.
> +  uint32_t length = aName.Length();
> +  const char* data = aName.Data();
> +  for (uint32_t i = 0; i < length; i++) {
> +    if (data[i] < 0x20 || data[i] > 0x7D || data[i] == '=') {

You know, it occurs to me that with the memchr() call below, checking for '=' here is redundant.

Comment 11

[Ralph Giles (:rillian)]

(In reply to Timothy B. Terriberry (:derf) from comment #10)

> You know, it occurs to me that with the memchr() call below, checking for
> '=' here is redundant.

That's correct. I thought it was better to validate that in case the method is called from a different context later.

Ready to land. Try push is green, but for some reason doesn't report on mochitest-1 on Mac. I'll reverify that locally on 10.7.

Comment 12

[Ralph Giles (:rillian)]

Local run of the media mochitests on MacOS 10.7 was green.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ralph Giles (:rillian) from comment #9)
> Pushed to try as https://tbpl.mozilla.org/?tree=Try&rev=ef0751b8a559

Green on Try.

https://hg.mozilla.org/integration/mozilla-inbound/rev/562ab305f2f3

Comment 14

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/562ab305f2f3

Comment 15

[Mike Kaply [:mkaply]]

Does this support streaming ogg as well?

http://dir.xiph.org/by_format/Ogg_Vorbis

Or does it only get the meta data at first load?

Comment 16

[Mike Kaply [:mkaply]]

Ignore my question, answer is yes. I tested it.

Comment 17

[Ralph Giles (:rillian)]

https://people.xiph.org/~giles/2013/metadata.html has some current examples, so you can see the metadata change.