OGG files can contain multiple chunks of meta data, called pages. The music player currently only supports reading the first page, but multiple pages should be supported as well. See STR in bug 916850 for an OGG file.

This is part of https://github.com/mozilla-b2g/gaia/pull/26660 Doesn't really block bug 907929. Makes it better

Comment on attachment 8537580 [details] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26836 This contain patch for bug 907929 as it depends on it.

Now rebased on master. Just a single commit.

Comment on attachment 8537580 [details] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26836 Looks good overall, but there are a few comments/concerns I had (see GitHub).

Note: this should go in 2.2 too.

Comment on attachment 8537580 [details] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26836 I have added tests. The only thing not changed is the change in core.js on which I commented. If still believe it is a concern, I can spend more time fixing it, even if in the current situation this is not a new problem.

(In reply to Hubert Figuiere [:hub] from comment #7) > I have added tests. The only thing not changed is the change in core.js on > which I commented. If still believe it is a concern, I can spend more time > fixing it, even if in the current situation this is not a new problem. I'm really worried that this will hurt startup performance when scanning music (keep in mind that we're forcing all users to rescan their media libraries in 2.2). I'd want to see some performance tests to be sure that this isn't regressing us before I r+ it. While it's possible that 256 KB is a better choice for all formats, I somewhat doubt it, and I suspect that we could go even smaller than 64 KB. I'm also confused as to why that change is necessary, since you should be able to get more data at any time, e.g. within OggMetadata.parse().

Comment on attachment 8537580 [details] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26836 cancelling review for now.

Comment on attachment 8537580 [details] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26836 I have addressed this issue you had with the initial blobview size. Also rebased. Let me know if you still have issues.

Comment on attachment 8537580 [details] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26836 It looks like there are some issues with how you're handling Promises in the patch. I also think we could simplify this by making it easier to fill an expanding buffer that could hold an entire comment field. That would eliminate the need for readContinuedComment. I have more detail about this on the PR. Thanks for working on this!

Comment on attachment 8537580 [details] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26836 I think comments are addressed. Updated the PR.

I'll make a test app first....

Comment on attachment 8537580 [details] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26836 cancelling review for now. I found a bug with the test app.

Comment on attachment 8537580 [details] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26836 The bug was in the test framework.

Comment on attachment 8537580 [details] Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26836 r-, but really only for the first comment over on GitHub. That part looks suspect...

I see wtf is going on and why I actually don't need to do that. I'll fix it and re-request a review. There might be more changes into the patch though. (I'll keep a separate commit)

Firefox OS is not being worked on