505158 - Resource selection/fetch algorithm fails when container has unsupported data format

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Matthew Gregan [:kinetik]]

<audio>
  <source src="fail_with_mp3.wav">
  <source src="win_with_vorbis.ogg">

Because Wave is a supported container, resource selection algorithm passes this file to the Wave decoder, which subsequently fails because the Wave contains MP3 data.

According to the spec, we should try the next source and successfully select win_with_vorbis.ogg in this case:

Resource fetch algorithm, step 3:
"If the media data can be fetched but is found by inspection to be in an unsupported format, or can otherwise not be rendered at all:

   1. The user agent should cancel the fetching process.
   2. Abort this subalgorithm, returning to the resource selection algorithm."

As these steps do not abort the resource selection algorithm, the following steps from step 4 of the Resource selection algorithm should execute:

"  5. Run the resource fetch algorithm with the absolute URL that resulted from
      resolving the URL given by the candidate element's src attribute relative 
      to candidate. If that algorithm returns without aborting this one, then
      the load failed.
   6. Queue a task to fire a simple event called error at the candidate element.
   7. Return to the step labeled search loop."

Right now, the decoders don't have a way to signal that they have failed to decode.  That's bug 481057.  Fixing this bug probably involves fixing bug 485288 as well (and they may well be dupes).

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

This was fixed by the load algorithm update in bug 485288.

Comment 2

[Matthew Gregan [:kinetik]]

This bug still exists.

Comment 3

[Matthew Gregan [:kinetik]]

Attachment: mp3 in wav

Comment 4

[Matthew Gregan [:kinetik]]

Attachment: vorbis in ogg

Comment 5

[Matthew Gregan [:kinetik]]

Attachment: testcase

Comment 6

[Matthew Gregan [:kinetik]]

Attachment: testcase

Comment 7

[Matthew Gregan [:kinetik]]

Testcase works in Opera 10.62 and Chrome 7.0.517.5 dev, fails in current Firefox 4 nightlies.

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

Ah, the problem here is that I misread the spec and assumed you must have a 'type' attribute in a <source> child. That is indeed not the case.

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 1

* Don't abort load algorithm if a media element's <source> child doesn't have a type attribute, as per the spec.
* Distinguish between decode failure when loading from a src attribute, and loading from <source> children. In the later case, don't set the error attribute, and try to load from the next source child as per the spec.
* Add test case to exercise this code.

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 2: video controls

* Setup the status fader if we've got no resource. This is so we can show the error cross when we've failed to load all resources.
* When we receive an "error" event, only show the error cross if all resources failed to load. This is if the video has its error attribute set, or if its networkState is NETWORK_NO_SOURCE. The later happens when we've tried to load all resources, and they all failed.

Without this change, we don't display controls for the following case:
<video>
  <source src="unsupported.mp4">
  <source src="supported.ogg>
</video>

This is because since the new load algorithm landed, we now receive an error event for each <source> we failed to load from, and the previous controls event-handler assumed we'd finished loading after receiving the first error event from the unsupported media case.

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

This should really block. We can't load videos with the following pattern:

<video>
  <source src="foo.mp4">
  <source src="bar.ogg">
</video>

We should be able to load that!

Comment 12

[Justin Dolske [:Dolske]]

Comment on attachment 476944 [details] [diff] [review]
Patch 2: video controls

>                         case "error":
...
>+                            if (this.video.error || this.video.networkState == this.video.NETWORK_NO_SOURCE) {

Could you add a comment here about error events being fired for <source>? Otherwise it's a bit odd to look at and wonder how we can get an error even without having video.error set.

Comment 13

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 1

Ready for check in.

Comment 14

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 2

With comment added as requested. Ready for check in.

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

http://hg.mozilla.org/mozilla-central/rev/a8657d2e743d
http://hg.mozilla.org/mozilla-central/rev/4874ca2efe6f