916850 - [Music] OGG meta-data parser fails

Status: RESOLVED FIXED

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

Description

[Thomas Zimmermann [:tzimmermann] [:tdz]]

The Music app fails to parse some meta data of type 'metadata_block_picture'.

STR:

  - download 'OGG Vorbis Audio' file from http://raumzeit-podcast.de/2011/12/02/rz029-herschel-weltraumteleskop/
  - open Music app

Expected:

  - downloaded file should be available for playing

Actual result:

  - the file isn't listed

Comment 1

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Github pull request

You'll need the patch from bug 916848 to mak eGecko recognize the .oga files.

Comment 2

[David Flanagan [:djf]]

Comment on attachment 805375 [details]
Github pull request

The patch does far more than what is described on bugzilla.  

Please explain/justify the changes to shared/js/blobview.js. They appear to change the public API of this shared file, so I'm very nervous about that.  Do you really need to make those changes to fix the ogg parser? 

If those blob view methods are only used by the the music metadata parser, maybe we could move them out of blob view and into the music metadata parser.

I think Jim is working on a music metadata patch that would be benefit from moving some of that stuff to metadata.js.

Also, music2 should never have checked in metadata_scripts.js.  That file should be dynamically generated in a local Makefile and should not be in github and should not need to be modified here.

Perhaps you can just file a separate bug for music2 to pick up the latest version of the metadata code from the original music app.

I haven't looked at the code carefully enough yet, so I'm not claiming that there is anything wrong with it.  Just that I'm nervious about changing it if other apps are relying on it.

I'm also uncertain about the change at line 174 of >= to >. Is the idea that we should be able to seek past the last byte of the file without causing an error? This deserves a comment, at least.

Comment 3

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi

(In reply to David Flanagan [:djf] from comment #2)
> Comment on attachment 805375 [details]
> Github pull request
> 
> The patch does far more than what is described on bugzilla.  

It really just fixes the parsing of some broken meta data. It's a larger patch because I didn't want to implement a workaround, but something more generic

> Please explain/justify the changes to shared/js/blobview.js. They appear to
> change the public API of this shared file, so I'm very nervous about that. 
> Do you really need to make those changes to fix the ogg parser? 

Some meta-data values in the linked .oga files seem broken. The idea of the patch is to first parse the key, and only parse the value if we're interested in it. For this to work, I need to tell the parser where the key ends. If there is a better approach to handle the problem, please let me know.

> If those blob view methods are only used by the the music metadata parser,
> maybe we could move them out of blob view and into the music metadata parser.

BlobView is responsible for parsing UTF-8, right? So I think the the method is at the correct place. Maybe a new method could be introduced that stops parsing at a certain character.

> I think Jim is working on a music metadata patch that would be benefit from moving some of that stuff to metadata.js.

Could you cc him please? Maybe he has an idea how to fix this problem.

> Also, music2 should never have checked in metadata_scripts.js.  That file
> should be dynamically generated in a local Makefile and should not be in
> github and should not need to be modified here.

That's definitely not caused by my patch. I was wondering about this, though.

> Perhaps you can just file a separate bug for music2 to pick up the latest
> version of the metadata code from the original music app.

The file blobview.js is an input for metadata_scripts.js. Because I changed the interface of BlobView, I had to make changes to music2 as well.

> I haven't looked at the code carefully enough yet, so I'm not claiming that
> there is anything wrong with it.  Just that I'm nervious about changing it
> if other apps are relying on it.

That's understandable. I've been careful to adapt all users of the interfaces I changed.

> I'm also uncertain about the change at line 174 of >= to >. Is the idea that
> we should be able to seek past the last byte of the file without causing an
> error? This deserves a comment, at least.

This change was necessary to make the patch work. But it's also seems more correct to me. If you have a buffer of n bytes length and you're at offset 0, reading n bytes will increment the offset to be equal to the buffer length. Seeking n bytes should give the same result. BlobView already contains the function 'advance', which is just a seek that works relative to the current position, and it supports the described behavior. Changing 'seek' just seemed natural; more like a bug fix.

I hope this help a bit. Please let me know if the general approach is sound, or how to best fix the problem. In the former case, I'd like to know what I need to change in the implementation. Thanks.

Comment 4

[David Flanagan [:djf]]

Thomas,

Sounds like that is a bug in the blobview.js seek method. Thanks for fixing it.

If you really need the ability to read UTF8 until a given delimiter, please add that as a new method without modifying the existing api for getUTF8Text or readUTF8Text.  If you do that, I'd suggest that you use a single JS character delimiter and don't try to handle the surrogate pair case.  Or maybe pass the delimiter as a codepoint (i.e. integer) rather than as a string.  As it is, I'd guess that the string creation and comparison on each time through the loop will make the blob view method measurably slower.

You haven't specified what the exception was, but I'm guessing it had somethign to do with trying to read binary data when expecting UTF8. If that was causing an exception, a patch that fixes readUTF8Text() to be more robust would be welcome.

What does the OGG spec say about the format of these commetn fields?  Is the text before the equals sign guaranteed to be ASCII?  If so, maybe you could just use a loop and blobview.readUnsignedByte() to find the equals sign.  Then use readASCIIText to read the name of the comment and readUTF8Text() to read the value, if you need to.  If you can get away without changes to blobview, I think that would be easier.

Comment 5

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi,

The exception is

> E/GeckoConsole(  450): Content JS ERROR at app://music.gaiamobile.org/js/metadata_scripts.js:634 in parseAudioMetadata/<: parseAudioMetadata: RangeError: argument 1 accesses an index that is out of range BlobView</BlobView.prototype.getUTF8Text@app://music.gaiamobile.org/js/metadata_scripts.js:262
> E/GeckoConsole(  450): BlobView</BlobView.prototype.readUTF8Text@app://music.gaiamobile.org/js/metadata_scripts.js:332
> E/GeckoConsole(  450): parseOggMetadata/<@app://music.gaiamobile.org/js/metadata_scripts.js:908
> E/GeckoConsole(  450): BlobView</BlobView.prototype.getMore@app://music.gaiamobile.org/js/metadata_scripts.js:73
> E/GeckoConsole(  450): parseOggMetadata@app://music.gaiamobile.org/js/metadata_scripts.js:876
> E/GeckoConsole(  450): parseAudioMetadata/<@app://music.gaiamobile.org/js/metadata_scripts.js:558
> E/GeckoConsole(  450): BlobView</BlobView.get/reader.onloadend@app://music.gaiamobile.org/js/metadata_scripts.js:55
> E/GeckoConsole(  450): Content JS WARN at app://music.gaiamobile.org/shared/js/mediadb.js:1672 in metadataError: MediaDB: error parsing metadata for /sdcard/rz033-energie-der-zukunft.opus : RangeError: argument 1 accesses an index that is out of range

Comment 6

[Thomas Zimmermann [:tzimmermann] [:tdz]]

The problem with these files is that parseOggMetadata in metadata.js computes a length of 4080 bytes for page 2 (i.e., the the page wi th the meta data). The actual key=value pair for 'metadata_block_picture' is 39655. Consequently the readUTF8 function fails.

Comment 7

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Github pull request

Ogg files are split into so-called pages. I found that the meta data in the affected Ogg files spans among multiple pages. The current code cannot handle this case easily, so I added a workaround. If the key-value pair is too long to fit into the current page, we simply stop parsing. This allows me to play the broken files and shouldn't have side effects on currently working files.

Comment 8

[Thomas Zimmermann [:tzimmermann] [:tdz]]

ping! Please review the attached patch.

Comment 9

[Thomas Zimmermann [:tzimmermann] [:tdz]]

ping! Would you please review the new patch.

Comment 10

[Dominic Kuo [:dkuo]]

Hi Thomas, I think djf is busy on the blockers and reviewing many patches for blockers(you should see his queue is kind of full), so you probably need to wait or find some other qualified reviewers if you want to land this soon.

Comment 11

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi Dominic,

Thanks for your answer. You also work on the Music app, right? Can you review this?

Comment 12

[Paul Adenot (:padenot)]

Comment on attachment 811060 [details]
Github pull request

David, stealing the review, as you seem to be very busy and we need to move forward, here.

Comment 13

[Paul Adenot (:padenot)]

Comment on attachment 811060 [details]
Github pull request

Thomas, I've added questions to the PR, re-request review when you have answers.

Comment 14

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi Paul

(In reply to Paul Adenot (:padenot) from comment #13)
> Comment on attachment 811060 [details]
> Github pull request
> 
> Thomas, I've added questions to the PR, re-request review when you have
> answers.

I added a return statement next to line 477, and a comment to explain what '4' is for. We cannot call the error callback before breaking, because this will mark the file as 'invalid', and it will not be shown in the media player. See metadataError in mediadb.js. I'll update the PR when the upload to github has finished.

Comment 15

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Github pull request

Comment 16

[Paul Adenot (:padenot)]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #14)
> Hi Paul
> 
> (In reply to Paul Adenot (:padenot) from comment #13)
> > Comment on attachment 811060 [details]
> > Github pull request
> > 
> > Thomas, I've added questions to the PR, re-request review when you have
> > answers.
> 
> I added a return statement next to line 477, and a comment to explain what
> '4' is for. We cannot call the error callback before breaking, because this
> will mark the file as 'invalid', and it will not be shown in the media
> player. See metadataError in mediadb.js. I'll update the PR when the upload
> to github has finished.

Makes sense, thanks. Can you open a bug for the multi-page metadata? Maybe if nobody has time to do it, we can mark it as mentored? Its self-contained and small enough to be interesting for a newcomer.

Comment 17

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Thanks for the review. I added bug 947884 for multi-page support.

Can you merge the github pull request? There are Travis failures, but I cannot see that they relate to my patch set. The previous patch passed the tests, and I only added some comments and a return statement.

Comment 18

[Paul Adenot (:padenot)]

I can't merge, but I would expect that Etienne can.

Comment 19

[Etienne Segonzac (:etienne)]

Redirecting to Rik since I'm unable to restart travis builds...

Comment 20

[Anthony Ricaud (:rik)]

I've restarted the gaia-ui-test because there's too many failures to land as is. The only unit test failure is a known intermittent so I haven't restarted it.

Comment 21

[Anthony Ricaud (:rik)]

Merged in https://github.com/mozilla-b2g/gaia/commit/46243a1ceed9df04a3614b228d9b60794c455610.