910576 - [B2G][Helix][Audio][tianhuajian]The libVorbis 1.0 ogg file can not played

Status: RESOLVED FIXED

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

Description

[tianhuajian]

User Agent: Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 6.1; Trident/4.0; SLCC2; .NET CLR 2.0.50727; .NET CLR 3.5.30729; .NET CLR 3.0.30729; Media Center PC 6.0; aff-kingsoft-ciba; .NET4.0C; .NET4.0E)

Steps to reproduce:

1. Push the ogg file A.ogg to sdcard by using adb, the A.ogg is encoded by libVorbis 1.0
2. Open the Music Player


Actual results:

The A.ogg can not be finded in playlist


Expected results:

The A.ogg can be displayed in playlist , and can be played by Music Player.

Comment 1

[lecky]

The test version is as following:
【Gaia commit ID*】: V1.1HD
【Gecko commit   The test version is as following:

Comment 2

[lecky]

 The test version is as following:
【Gaia commit ID*】: V1.1HD
【Gecko commit ID*】:  1.1.0 HD

Comment 3

[Dominic Kuo [:dkuo]]

(In reply to tianhuajian from comment #0)
> 1. Push the ogg file A.ogg to sdcard by using adb, the A.ogg is encoded by
> libVorbis 1.0

Using adb to push files into the device won't trigger the rescanning of music app,
and it's also not a normal way for users to put they media content into the internal/external storages,
so if this issue is reproducible with step 1, I would say this is INVALID or WONTFIX.

> 2. Open the Music Player

I guess the music app is already launched, make sure it's not open yet by holding the home button.
if it does, this might be a bug.

Can you please attach the test file or link? thanks.

Comment 4

[lecky]

Attachment: ogg-001.part1.rar

The ogg file which can not displayed in the playing list

Comment 5

[lecky]

Attachment: ogg-001.part2.rar

The ogg test file

Comment 6

[lecky]

> 1. Push the ogg file A.ogg to sdcard by using adb, the A.ogg is encoded by
> libVorbis 1.0
After push the A.ogg to the device, I try the two methods to check :
method 1: close the Music Player app and reopen it
method 2: close the device and repopen it .
Both the two method can reproduce the issue.
And I also push other music file as the same step.
These music file can be displayed in the playing list, and can play normally.

Comment 7

[lecky]

> 2. Open the Music Player

I am sure the Music Player has opened .
I have attached the ogg music file.

Comment 8

[Dominic Kuo [:dkuo]]

I took a test and found the current metadata parser does not recognise this ogg file, need to figure out what's the difference between this ogg and the others.

Comment 9

[lecky]

(In reply to Dominic Kuo [:dkuo] from comment #8)
> I took a test and found the current metadata parser does not recognise this
> ogg file, need to figure out what's the difference between this ogg and the
> others.

This ogg file is encodered by libVorbis 1.0, the others is not encoder by this encoder lib .
Do you mean FFOS can not support this file format in v1.1?
And in which version FFOS can fully support this format?

Comment 10

[Wayne Chang [:wchang]]

Hi Marco,

Can someone check the support for this?

Removing nomination assuming this is by default not supported, please renom if this is supposed to be supported and this is a valid bug.

Comment 11

[Wayne Chang [:wchang]]

I am not sure if this recognition omission occurred at gecko or gaia/music app.

Comment 12

[Wayne Chang [:wchang]]

mid-air clashed. adding ni?dkuo for visibility.

thanks dominic.

Comment 13

[Dominic Kuo [:dkuo]]

(In reply to lecky from comment #9)
> Do you mean FFOS can not support this file format in v1.1?
> And in which version FFOS can fully support this format?

After I re-tested again and found we do support the attached ogg. I put that ogg on web and use Browser app to open it and it works. So the reason why the music app cannot play it is because the metadata parser does not recognise the ogg, then music app will not display a unrecognisable file.

So this is a gaia bug in the metadata parser of music app.

Comment 14

[Marco Chen [:mchen]]

Hi Dominic,

Thanks for your effort to check this bug first.
Since it is a Gaia issue, I remove the NI.

Comment 15

[lecky]

(In reply to Dominic Kuo [:dkuo] from comment #13)
So this is a gaia bug in the metadata parser of music app.

Hi Dominic,
Can you give us a detail fix plan of this issue?
Thanks

Comment 16

[Dominic Kuo [:dkuo]]

I haven't got a plan to fix this because this looks like a general issue on parsing the metadata of ogg files, and I need to read the spec to see what's still missing in our current parser, so if we want to speed up the process, probably I will need some help from you.

Is this the only ogg that you can reproduce this issue? it will be very helpful if you can help to describe what's the differences between the attached one and the others, thanks.

Comment 17

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: PR for 910576

Submitting initial work on this -- there was an issue in BlobView's readUTF8String method not handling the parsing correctly, with a TODO message to use StringEncoder API's anyway (bug 764234).

It appears that this ogg file only has one tag, ("������=Audio Converter"), with the actual values of the tag name being U+fffd, or the special unicode replacement character. What should this be in the first place?

Going to make more vorbis files with 1.0 and see how that plays out, but would like to know what the meta for this vorbis file should be in the first place. Where do the tests for this live? The BlobView itself could use unit tests, and the music app doesn't seem to have any tests regarding metadata decoding. If you could point me in the right direction, I'd love to dig in more!

Comment 18

[David Flanagan [:djf]]

Comment on attachment 814590 [details]
PR for 910576

Jim,

You've worked on this code most recently, so I'm passing this feedback request on to you.

Comment 19

[Jim Porter (:squib)]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #17)
> ...the music app doesn't seem to have any tests regarding metadata decoding.

Unit tests for metadata decoding are Coming Soon(TM). I'm really close to landing a huge change to ID3 parsing (with tests!) and then I'll get started on some Ogg Vorbis tests.

See bug 841949.

Comment 20

[Jim Porter (:squib)]

Comment on attachment 814590 [details]
PR for 910576

I commented on the PR. Overall, this is a big improvement, but we can still clean things up further.

Comment 21

[GitHub Autolander Bot]

Attachment: [gaia] jimporter:blobview-textdecoder > mozilla-b2g:master

Comment 22

[Hubert Figuiere [:hub]]

Do you think we can get a unit test for this one?

Comment 23

[Jim Porter (:squib)]

Yes, that's why I haven't requested review yet.

Comment 24

[Jim Porter (:squib)]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #17)
> It appears that this ogg file only has one tag, ("������=Audio Converter"),
> with the actual values of the tag name being U+fffd, or the special unicode
> replacement character. What should this be in the first place?

This is a really old comment, but I decided to go through this file with a hex editor. The issue is that, *once again*, some broken encoder decided to spit out non-Unicode text into a file that's specced to only accept Unicode. I'm willing to grudgingly accept this for ID3 (see bug 850520), but absolutely not for Ogg Vorbis.

Luckily, to fix this specific issue, we have to do... absolutely nothing. This file scans correctly on master without issue.* Instead, I'll take this opportunity to be a little less strict about non-Unicode in our parsers so that we just get mojibake instead of a completely empty metadata field. Tests for that forthcoming.

With this bug fixed, I think the only other invalid files we'll want to support are covered in bug 850520, so triagers can feel free to WONTFIX any (non-regression) bug that turns out to be a result of someone trying to play an invalid file. If people would find it helpful, I can write up a more-thorough explanation of what exactly we want to parse, and what we'll consider unreadable.

* I saw some scanning hangs, but they don't occur with just this file on the device, so that's probably a different bug.

Comment 25

[Jim Porter (:squib)]

Comment on attachment 8670034 [details] [review]
[gaia] jimporter:blobview-textdecoder > mozilla-b2g:master

Ok, this should be good now. The main change is that we use TextDecoder for all the text-reading functions in BlobView. This means that improperly-encoded text won't cause us to completely fail to parse a metadata field. The bad characters will just get the Unicode replacement char instead.

Using TextDecoder had the side effect of breaking some Gallery code, since it was reading "ASCII", but what it really wanted was the raw bytes *not* converted to the corresponding Unicode codepoints. I've fixed this by fetching a Uint8Array instead (and converting to a String in the Gallery code to make comparisons easier).

I also standardized the naming for BlobView to use "ASCII" instead of a mix of "ASCII" and "Latin1". Technically, "Latin1" is probably the more-correct name, but it would mean changing the calls in many more places.

I also added a test for parsing invalid UTF-8 in the music app. No tests for ASCII and UTF-16, since I don't think TextDecoder cares about invalid text for those encodings. At least, I couldn't trigger a failure.

Comment 26

[David Flanagan [:djf]]

Comment on attachment 8670034 [details] [review]
[gaia] jimporter:blobview-textdecoder > mozilla-b2g:master

This mostly looks good, but see github for my comments and suggestions.

I don't like the semantics of getASCIIText changing so much that you had to modify the image_size.js code.  If you want to rename all the ascii and latin-1 method names so that they more accurately use "BinaryString" in their name, that might be a good idea.  But I'm not convinced that we actually need a formal TextDecoder('ascii') solution.

Also, your handling of UTF-16 adds an exception that the old implementation did not throw, and that could be a source of errors for malformed metadata.

Overall, I'm glad you're doing this.  But it looks like you're taking on more blobview changes than strictly necessary to fix your utf8 bug and have made it a trickier patch to review.

Comment 27

[Jim Porter (:squib)]

Comment on attachment 8670034 [details] [review]
[gaia] jimporter:blobview-textdecoder > mozilla-b2g:master

I've renamed the functions, so now there are four encodings: "binary" (what we used to call ASCII), latin-1, utf-8, and utf-16. We do need latin-1 because ID3 can contain text encoded in latin-1, and we'd get mojibake if we used a "binary" encoding. At least, we were getting different results using latin-1 vs "binary", and I trust TextDecoder more for this.

Comment 28

[David Flanagan [:djf]]

Comment on attachment 8670034 [details] [review]
[gaia] jimporter:blobview-textdecoder > mozilla-b2g:master

There are a few nits that I'd like you to fix. See github. But overall this looks great. Thanks.

Comment 29

[Jim Porter (:squib)]

Landed with comments addressed: https://github.com/mozilla-b2g/gaia/commit/34ae4e8a890a9fa6fd1e1b4d4d0491a697d0ee82