Closed
Bug 778050
Opened 12 years ago
Closed 12 years ago
Support Opus tag metadata
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: rillian, Assigned: rillian)
References
Details
Attachments
(1 file, 3 obsolete files)
15.84 KB,
patch
|
derf
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•12 years ago
|
||
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.
Assignee: nobody → giles
Attachment #660594 -
Flags: review?(tterribe)
Assignee | ||
Comment 2•12 years ago
|
||
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.
Assignee | ||
Comment 3•12 years ago
|
||
Comment on attachment 660608 [details] [diff] [review] Support Opus tag metadata Update patch to include a test case.
Attachment #660608 -
Attachment description: Support Opus tag metadata - parsed comments from the OpusTags header in → Support Opus tag metadata
Attachment #660608 -
Flags: review?(tterribe)
Assignee | ||
Updated•12 years ago
|
Attachment #660594 -
Attachment is obsolete: true
Attachment #660594 -
Flags: review?(tterribe)
Assignee | ||
Comment 4•12 years ago
|
||
Update to fix a debug message typo. Sorry for the noise. pushed to try as https://tbpl.mozilla.org/?tree=Try&rev=803d96343a8b
Attachment #660608 -
Attachment is obsolete: true
Attachment #660608 -
Flags: review?(tterribe)
Attachment #660613 -
Flags: review?(tterribe)
Assignee | ||
Comment 5•12 years ago
|
||
Try push is green. Orange is three timeouts in unrelated test sections.
Comment 6•12 years ago
|
||
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?
Attachment #660613 -
Flags: review?(tterribe) → review+
Assignee | ||
Comment 7•12 years ago
|
||
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.
Assignee | ||
Comment 8•12 years ago
|
||
Updating in response to comments. I moved things to nsOggCodecState instead of into nsOggReader. That seemed to make more sense to me.
Attachment #660613 -
Attachment is obsolete: true
Attachment #661439 -
Flags: review?(tterribe)
Assignee | ||
Comment 9•12 years ago
|
||
Pushed to try as https://tbpl.mozilla.org/?tree=Try&rev=ef0751b8a559
Comment 10•12 years ago
|
||
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.
Attachment #661439 -
Flags: review?(tterribe) → review+
Assignee | ||
Comment 11•12 years ago
|
||
(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.
Keywords: checkin-needed
Assignee | ||
Comment 12•12 years ago
|
||
Local run of the media mochitests on MacOS 10.7 was green.
Comment 13•12 years ago
|
||
(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
Flags: in-testsuite+
Keywords: checkin-needed
Comment 14•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/562ab305f2f3
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Comment 15•11 years ago
|
||
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•11 years ago
|
||
Ignore my question, answer is yes. I tested it.
Assignee | ||
Comment 17•11 years ago
|
||
https://people.xiph.org/~giles/2013/metadata.html has some current examples, so you can see the metadata change.
You need to log in
before you can comment on or make changes to this bug.
Description
•