Closed Bug 505158 Opened 11 years ago Closed 9 years ago

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

Categories

(Core :: Audio/Video, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: kinetik, Assigned: cpearce)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 3 obsolete files)

<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).
This was fixed by the load algorithm update in bug 485288.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
This bug still exists.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached audio mp3 in wav
Attached audio vorbis in ogg
Attached file testcase (obsolete) —
Attached file testcase
Attachment #476043 - Attachment is obsolete: true
Testcase works in Opera 10.62 and Chrome 7.0.517.5 dev, fails in current Firefox 4 nightlies.
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.
Attached patch Patch 1 (obsolete) — Splinter Review
* 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.
Assignee: nobody → chris
Status: REOPENED → ASSIGNED
Attachment #476941 - Flags: review?(roc)
Attached patch Patch 2: video controls (obsolete) — Splinter Review
* 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.
Attachment #476944 - Flags: review?(dolske)
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!
blocking2.0: --- → ?
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.
Attachment #476944 - Flags: review?(dolske) → review+
Attached patch Patch 1Splinter Review
Ready for check in.
Attachment #476941 - Attachment is obsolete: true
Attachment #478678 - Flags: review+
Attached patch Patch 2Splinter Review
With comment added as requested. Ready for check in.
Attachment #476944 - Attachment is obsolete: true
Attachment #478679 - Flags: review+
Keywords: checkin-needed
Whiteboard: [needs landing]
Blocks: 600806
Blocks: 595009
http://hg.mozilla.org/mozilla-central/rev/a8657d2e743d
http://hg.mozilla.org/mozilla-central/rev/4874ca2efe6f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
Whiteboard: [needs landing]
Blocks: 527822
You need to log in before you can comment on or make changes to this bug.