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

NEW
Unassigned

Status

()

P3
critical
3 years ago
3 years ago

People

(Reporter: justindarc, Unassigned)

Tracking

Trunk
ARM
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(tracking-b2g:backlog, firefox44 affected)

Details

(Whiteboard: [platform])

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
(Reporter)

Comment 1

3 years ago
[Blocking Requested - why for this release]: This is blocking Bug 1149930 which is a v2.5+ blocker.
Blocks: 1149930
Severity: normal → critical
blocking-b2g: --- → 2.5?
Flags: needinfo?(cpearce)
(Reporter)

Comment 2

3 years ago
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.
Flags: needinfo?(cpearce) → needinfo?(ajones)

Updated

3 years ago
blocking-b2g: 2.5? → 2.5+
OS: Unspecified → Gonk (Firefox OS)
Priority: -- → P1
Hardware: Unspecified → ARM

Updated

3 years ago
Whiteboard: [platform]
(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?
(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.
(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.
(Reporter)

Comment 6

3 years ago
(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.
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.
(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #3)
> Is this a proper fix?

Nobody seems to be answering this question.
Flags: needinfo?(ajones)
(Reporter)

Comment 10

3 years ago
(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.
Any update on this?
(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.
(Reporter)

Comment 13

3 years ago
(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.
(Reporter)

Comment 14

3 years ago
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)
> 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 :)
blocking-b2g: 2.5+ → ---
tracking-b2g: --- → backlog
(Reporter)

Comment 16

3 years ago
(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.
Cool, thanks.  I clarified the summary here but feel free to improve it further if necessary.
Summary: [HTMLAudioElement] Error code 3 (MEDIA_ERR_DECODE) is thrown when no output device is present → [HTMLAudioElement] Don't throw Error code 3 (MEDIA_ERR_DECODE) when no output device is present
Priority: P1 → P2
Mass change P2 -> P3
Priority: P2 → P3
You need to log in before you can comment on or make changes to this bug.