855647 - [Music] support opus in ogg

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Music, defect)

Description

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: sample opus in ogg file

The mediaRecorder would record in opus format, but it seems music app can't parsing the opus in ogg music files.
logs. 
03-27 19:14:18.777 E/GeckoConsole(  547): Content JS WARN at app://music.gaiamobile.org/shared/js/mediadb.js:1332 in metadataError: MediaDB: error parsing metadata for 6005.ogg : Error: advance past end of buffer
03-27 19:14:18.777 E/GeckoConsole(  547): Content JS WARN at app://music.gaiamobile.org/shared/js/mediadb.js:1332 in metadataError: MediaDB: error parsing metadata for 6005.ogg : malformed ogg comment packet

Comment 1

[Timothy B. Terriberry (:derf)]

It looks like parseOggMetadata() is hard-coded to only support Vorbis:
https://github.com/mozilla-b2g/gaia/blob/master/apps/music/js/metadata.js#L417

I think just modifying the check at line 440 to look for 'OpusTags' in addition to '\003vorbis' would be enough to make this work.

See the format for Opus metadata at <http://tools.ietf.org/html/draft-ietf-codec-oggopus-00#section-5.2>.

Comment 2

[Alex Keybl [:akeybl]]

Product - please re-nominate if this breaks a user story (we're not aware of one).

Comment 3

[Chris Lee [:clee]]

This does not block leo.  Thanks.

Comment 4

[Sandip Kamat]

We need this targeted for v1.2

Comment 5

[Timothy B. Terriberry (:derf)]

Attachment: [music] Add Opus support

I've got a device for a few days I can test things on, so I went ahead and wrote a simple patch for this.

Comment 6

[Shelly Lin [:shelly]]

Attachment: Another ogg+opus file

Hi, this is a mush more recent ogg file generated by our MediaEncoder (bug 842243). Hope that it helps.

Comment 7

[Dominic Kuo [:dkuo]]

Comment on attachment 740584 [details] [diff] [review]
[music] Add Opus support

Timothy,

This patch looks good, and with this patch music app is able to parse the audio files encode with opus, and their metadata are also correctly parsed.

David has wrote the metadata parser of music app, so I feel like he should be notified that metadata.js will be modified. Setting feedback to David, thanks.

Comment 8

[David Flanagan [:djf]]

Comment on attachment 740584 [details] [diff] [review]
[music] Add Opus support

Thanks for flagging this for me, Dominic. The patch looks great. How nice that it was so easy to support a new type!

Comment 9

[Timothy B. Terriberry (:derf)]

Okay, last B2G patch I did, someone else landed for me. What do I need to do to get this one checked in?

Comment 10

[Dominic Kuo [:dkuo]]

(In reply to Timothy B. Terriberry (:derf) from comment #9)
> Okay, last B2G patch I did, someone else landed for me. What do I need to do
> to get this one checked in?

You need a github account to open a pull request to gaia repo, but I can do this for you.

Comment 11

[Dominic Kuo [:dkuo]]

Landed on master: 153f6805839ccfcd4b1064568377ef26bacf2960

Comment 12

[Timothy B. Terriberry (:derf)]

Comment on attachment 740584 [details] [diff] [review]
[music] Add Opus support

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Not a regression.
User impact if declined: Users won't be able to play Opus files (including those created by the Recording API once it lands) in the music app.
Testing completed: Tested locally on a unagi device, landed on master.
Risk to taking this patch (and alternatives if risky): Very low: it's a simple, self-contained patch that makes the existing metadata reader work. All the code to actually play back Opus files is already available to web content via the <audio> tag.
String or UUID changes made by this patch: None.

Comment 13

[Alex Keybl [:akeybl]]

Adding qawanted to check for regressions before considering this bug for uplift.

Comment 14

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Will verify tomorrow and test around the fix:

Reviewing code change:
https://github.com/mozilla-b2g/gaia/commit/153f6805839ccfcd4b1064568377ef26bacf2960

Comment 15

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Found a nice site to convert audio file types : http://audio.online-convert.com/convert-to-opus

Also used various other types and a corrupt ogg file.  Opus plays back, Vorbis plays back, Theora opens up in video app, all supported audio still shows, nonsupported audio does not show in the music app.  

Music app and volume control issue noted in Bug 858745
Used files from : http://people.mozilla.com/~nhirata/B2G/Soundfiles.zip

Verified with :
"gecko" revision="84f4c17f1605"
"gaia" revision="4e7d63a83508caa391c4db164c3f68422d9ca5b6"
Build ID: 2013-05-09-07-02-05
MC/master build
Unagi

Comment 16

[Ryan VanderMeulen [:RyanVM]]

John, can you uplift to v1-train please?

Comment 17

[John Ford [:jhford] CET/CEST Berlin Time]

[v1-train 7cc7bd3]