1215272 - [HTMLAudioElement] Don't throw Error code 3 (MEDIA_ERR_DECODE) when no output device is present

Status: RESOLVED WORKSFORME

(Core :: Audio/Video: Playback, defect, P3)

Description

[Justin D'Arcangelo [:justindarc]]

When trying to land the patch for Bug 1149930 for the Gaia Music app, we ran into integration test failures on TaskCluster. The test failures seem to be caused by the fact that TaskCluster does not have any audio output device. The patch in Bug 1149930 intends to skip over corrupt audio files and simply adds an event listener for the "error" event and attempts to skip to the next song in the queue. On TaskCluster, valid OGG and MP3 files are throwing the "error" event with error code 3 (MEDIA_ERR_DECODE) which seems incorrect. It would make more sense to throw error code 1 (MEDIA_ERR_ABORTED) or to simply introduce a new error code for cases where a lack of hardware is preventing audio playback.

In addition to task cluster, this exact same issue can be reproduced when running the Gaia integration tests on a local Linux machine using the `PULSE_AUDIO=":"` flag as documented here:

https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/Automated_testing/Gaia_integration_tests#Running_the_tests_in_the_background_quietly

Comment 1

[Justin D'Arcangelo [:justindarc]]

[Blocking Requested - why for this release]: This is blocking Bug 1149930 which is a v2.5+ blocker.

Comment 2

[Justin D'Arcangelo [:justindarc]]

Actually, after looking at msIDOMMediaError.idl, I'm not sure MEDIA_ERR_ABORTED is correct either. We really either need a new error code to handle this case -or- do not throw an error at all.

Comment 3

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Justin D'Arcangelo [:justindarc] from comment #2)
> Actually, after looking at msIDOMMediaError.idl, I'm not sure
> MEDIA_ERR_ABORTED is correct either. We really either need a new error code
> to handle this case -or- do not throw an error at all.

Is this a proper fix?

The way I read it is that we're not really going to be testing anything properly unless we have a dummy audio device. Why don't we just disable the test?

Comment 4

[Jim Porter (:squib)]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #3)
> The way I read it is that we're not really going to be testing anything
> properly unless we have a dummy audio device. Why don't we just disable the
> test?

I can't comment definitively on this, since Treeherder is refusing to show me test results right now, but I assume the failing test was in the music app. Most (all?) of those tests are there for a reason, and I'd be extremely hesitant to disable any existing tests, especially given that we've been told that the next release cycle should be entirely devoted to adding even *more* tests.

In general, I think disabling tests should be a measure of last resort. If it's feasible to fix this bug while keeping the tests enabled, we should do it.

Comment 5

[Hubert Figuiere [:hub]]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #3) 
> The way I read it is that we're not really going to be testing anything
> properly unless we have a dummy audio device. Why don't we just disable the
> test?

We are testing playback and a flow of "perations" in the Music app. We don't test the output of that playback but playing a valid file should just "work" and not error, so that we can test that the application work, also that we can test that errors for actually invalid files getting handled properly - tests started failing when we started handling errors. As it is right now, we can't test that properly.

Disabling the test on automation is not even an acceptable workaround.

Comment 6

[Justin D'Arcangelo [:justindarc]]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #3)
> (In reply to Justin D'Arcangelo [:justindarc] from comment #2)
> > Actually, after looking at msIDOMMediaError.idl, I'm not sure
> > MEDIA_ERR_ABORTED is correct either. We really either need a new error code
> > to handle this case -or- do not throw an error at all.
> 
> Is this a proper fix?
> 
> The way I read it is that we're not really going to be testing anything
> properly unless we have a dummy audio device. Why don't we just disable the
> test?

As Hub mentioned, its not just one test. We would have to disable over 50% of the Music app integration tests.

Comment 7

[Jim Porter (:squib)]

I'd also like to add that testing "errors for actually invalid files getting handled properly" is extremely important for the Music app since, to my knowledge, Gecko doesn't have (complete?) test coverage here. We recently had to spend several weeks diagnosing a Gecko regression that caused invalid audio files to stall the Music app. Having tests to help us exercise our audio handling in the Music app will hopefully prevent that from happening again.

Comment 8

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #3)
> Is this a proper fix?

Nobody seems to be answering this question.

Comment 9

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Looks like we need to do something like this http://earthwithsun.com/questions/344760/how-to-create-a-dummy-sound-card-device-in-linux-server

Comment 10

[Justin D'Arcangelo [:justindarc]]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #8)
> (In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #3)
> > Is this a proper fix?
> 
> Nobody seems to be answering this question.

Possibly. It seems incorrect to me to dispatch a "decode" error when the error has to do with playback/output. It would be beneficial to differentiate between the two from the application level as we could tell if a media file is corrupt or if the hardware we're running on simply is unable to play audio.

Comment 11

[Hema Koka [:hema]]

Any update on this?

Comment 12

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Hema Koka [:hema] from comment #11)
> Any update on this?

The proper solution is to fix the test infrastructure so we've got a dummy sound device. Someone needs to follow up with whoever manages the test infrastructure.

Comment 13

[Justin D'Arcangelo [:justindarc]]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #12)
> (In reply to Hema Koka [:hema] from comment #11)
> > Any update on this?
> 
> The proper solution is to fix the test infrastructure so we've got a dummy
> sound device. Someone needs to follow up with whoever manages the test
> infrastructure.

Agreed, but I would still argue that "MEDIA_ERR_DECODE" is semantically incorrect as an error code for describing the situation where audio can't be played back due to a lack of hardware. I would still like to see an additional error code here if possible.

Comment 14

[Justin D'Arcangelo [:justindarc]]

Actually, after looking at the spec, I'm not sure its possible to have an additional error code:

http://www.w3.org/TR/html5/embedded-content-0.html#error-codes

I was under the impression that the error codes for media elements were up to the vendor to implement. It seems that the error codes that Gecko implements are exactly those that are specified in the W3C spec, so I don't think we can just add a new code. My bad.

So, with that being said, I still don't think that any of the available error codes apply to this particular situation. These error codes seem to be related to problems with the the source content itself and don't really have anything to do with hardware errors like the one we're seeing.

In this case, I think the proper thing might just be to *not* throw any errors if audio can't be played back due to lack of hardware. (note: this is my recommendation *in addition to* fixing the problems with our VM setup in our test infrastructure)

Comment 15

[Andrew Overholt [:overholt]]

> In this case, I think the proper thing might just be to *not* throw any
> errors if audio can't be played back due to lack of hardware. (note: this is
> my recommendation *in addition to* fixing the problems with our VM setup in
> our test infrastructure)

Can you file a separate bug for this, please?

If this bug is now going to be used solely for the test infrastructure changes required, let's clarify the summary.

Regarding the infrastructure changes required, I don't think we should block 2.5 on this.  A brief discussion with Mahe and a few others seems to indicate at least a bit of agreement so I'm removing 2.5+ here.  If people disagree, as always please feel free to re-nom and give some reasoning :)

Comment 16

[Justin D'Arcangelo [:justindarc]]

(In reply to Andrew Overholt [:overholt] from comment #15)
> > In this case, I think the proper thing might just be to *not* throw any
> > errors if audio can't be played back due to lack of hardware. (note: this is
> > my recommendation *in addition to* fixing the problems with our VM setup in
> > our test infrastructure)
> 
> Can you file a separate bug for this, please?
> 
> If this bug is now going to be used solely for the test infrastructure
> changes required, let's clarify the summary.
> 
> Regarding the infrastructure changes required, I don't think we should block
> 2.5 on this.  A brief discussion with Mahe and a few others seems to
> indicate at least a bit of agreement so I'm removing 2.5+ here.  If people
> disagree, as always please feel free to re-nom and give some reasoning :)

No problem. Bug 1215257 was already filed to track the infrastructure changes needed.

Comment 17

[Andrew Overholt [:overholt]]

Cool, thanks.  I clarified the summary here but feel free to improve it further if necessary.

Comment 18

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Mass change P2 -> P3

Comment 19

[Andrei Purice]

Hey Justin,
Can you still reproduce this issue or should we close it?

Comment 20

[Justin D'Arcangelo [:justindarc]]

This was when working on Firefox OS. I have no idea if this is still an issue. You can probably close it.

Comment 21

[Andrei Purice]

Marking this as Resolved > Worksforme as per reporter's last response.
If anyone can still reproduce this issue re-open it or file a new bug.