Closed Bug 534571 Opened 15 years ago Closed 14 years ago

Calling load() on an <audio> or <video> element doesn't work if using <source> elements.

Categories

(Core :: Audio/Video, defect)

Product:
Core
Component:
Audio/Video
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

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

People

(Reporter: eriksjunk, Assigned: cpearce)

References

(
URL
)

Details

Attachments

(2 files, 1 obsolete file)

Patch 1: reset HTMLMediaElement::mSourcePtr
3.62 KB, patch
roc
: review+
Details | Diff | Splinter Review
Patch 2 v2: Firing timeupdate instead.
3.08 KB, patch
roc
: review+
Dolske
: review+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2b4) Gecko/20091124 Firefox/3.6b4 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2b4) Gecko/20091124 Firefox/3.6b4 (.NET CLR 3.5.30729)

When load() is invoked on an <audio> element whose source is specified using <source> elements, the audio fails to load.

Reproducible: Always

Steps to Reproduce:
1. Create a page with an <audio> tag, specifying its source with a <source> tag
2. Invoke load() on the element
Actual Results:  
An "X" appears above the controls, indicating that the loading failed.

Expected Results:  
Firefox loads the file specified in the <source> element.

The attached URL is a simple page that is supposed to let me change the track using a drop down.  It works perfectly in Chrome and Safari, but not in Firefox.
The linked URL doesn't resolve for me. Can you paste a link to the actual audio file? Is it an Ogg or a Wav file? What mime type is it served with?
Oops, my mistake.  The link should work, now.  It's an ogg, served as "audio/ogg" with the type set to "audio/ogg; codecs=vorbis" in the HTML.  It works on the initial page load, but it stops working after load is called.  I added a button that just calls "load()" on the element, to verify that the problem doesn't have to do with changing the sources.  Also, I tested with only one source tag (the example has three), and the same problem occurred.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Calling load() on an <audio> element doesn't work if using <source> elements. → Calling load() on an <audio> or <video> element doesn't work if using <source> elements.
Tested in: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a4pre) Gecko/20100403 Minefield/3.7a4pre

I've just tried the following code:

<!DOCTYPE html>
<html lang="en">
  <head>
    <title>SOURCE ELEMENT TESTS</title>
  </head>
  <body>
      <video controls>
        <source src="HelloWorld.ogv" type="video/ogg; codecs='theora,vorbis">
        <source src="HelloWorld.mp4" type="video/mp4; codecs='avc1.42E01E, mp4a.40.2">
      </video>
      
      <p>Video supports the following MIME types:
        <ul>
          <script type="text/javascript">
            var types = new Array();
            types[0] = "video/ogg";
            types[1] = "video/ogg; codecs='theora,vorbis'";
            types[2] = "video/mp4";
            types[3] = "video/mp4; codecs='avc1.42E01E, mp4a.40.2'";
        
            // get video element
            var video   = document.getElementsByTagName("video")[0];

            // test types
            for (i=0; i<types.length; i++) {
              var support = video.canPlayType(types[i]);
              if (support == "") support="none";
              document.write("<li><b>"+types[i]+"</b> : "+support+"</li>");
            }
          </script>
      </ul>
    </p>

  </body>
</html>


The interesting thing is that Minefield 3.7a4pre reports:

# video/ogg : maybe
# video/ogg; codecs='theora,vorbis' : none

This means that when you provide a codecs parameter, it seems to completely fail. This is in fact counter-intuitive to what the HTML5 specification says:

"Generally, a user agent should never return "probably" if the type doesn't have a codecs parameter."

I would expect in the more specific case of "video/ogg; codecs='theora,vorbis'" that Firefox returns "probably".
(In reply to comment #4)
> I would expect in the more specific case of "video/ogg; codecs='theora,vorbis'"
> that Firefox returns "probably".

The error you are seeing is due to using single quotes in the 'codec' part of the mime type. This is not valid. You need to use double quotes, or no quotes at all:

  types[1] = "video/ogg; codecs=theora,vorbis";

Once you make that change you'll see that 'probably' is reported.

Please see RFC 4281 for details.
Here's a pretty well-reduced test case I whipped up:
http://www.southeasthornworkshop.org/greg/audio-test

Broken on 3.6.3 and Gecko/20100502 Minefield/3.7a5pre
Fix is pretty simple. The first time we load a media element, we create a range of the child <source> elements (nsHTMLMediaElement::mSourcePtr). In nsHTMLMediaElement::LoadFromSourceChildren() we iterate over those source elements looking for a valid resource. We do this by calling GetNextSource(), which uses the range's start offset to denote which of the children it's up to, incrementing it each time. The second time we call load(), we don't reset the range to the first child, so GetNextSource() assumes we don't have any children. The fix is to destroy the range when we start a new load, we then consider all valid source children as candidates for the every load.

Patch includes backend independent test case.
Assignee: nobody → chris
Attachment #458245 - Flags: review?(roc)
The controls don't handle the testcase in this bug's URL correctly. When we load a video from a source element a second time, the controls don't reset the current playback position to 0, and we don't reset the play button to paused state (i.e. displaying the play button). This patch fixes that.
Attachment #458246 - Flags: review?(dolske)
(In reply to comment #8)
> Created attachment 458246 [details] [diff] [review]
> Patch 2: fix controls to display correctly on second load

It seems like the media element should fire a timeupdate event in this case, but I'm not able to find anything in the spec to back that up.
I agree that timeupdate should be fired, and if the spec doesn't say that, we should ask for it to be added.
Achieves the same as previous patch 2, but fires a timeupdate event when we start a load in order to reflect the time change in the UI. I'll request an additional review for controls change from dolske.
Attachment #458246 - Attachment is obsolete: true
Attachment #458255 - Flags: review?(roc)
Attachment #458246 - Flags: review?(dolske)
Comment on attachment 458255 [details] [diff] [review]
Patch 2 v2: Firing timeupdate instead.

Requesting additional review from dolske for the changes to videocontrols.xml.
Attachment #458255 - Flags: review?(dolske)
(In reply to comment #10)
> I agree that timeupdate should be fired, and if the spec doesn't say that, we
> should ask for it to be added.


I agree, but maybe it's already implicitly there.

As part of the load(), an update to the @startTime (or the earliest possible position is implied, and a timeupdate is implied upon change of @startTime. So, that may imply it.

We could, however, ask to have it explicitly added in the spec.
Can we make this blocking2.0? It would be good to get this checked in, as it's a simple low risk bug fix with test.
blocking2.0: --- → ?
Comment on attachment 458255 [details] [diff] [review]
Patch 2 v2: Firing timeupdate instead.

>                         case "loadstart":
>                             this.maxCurrentTimeSeen = 0;
>                             this.statusIcon.setAttribute("type", "throbber");
>                             this.isAudioOnly = (this.video instanceof HTMLAudioElement);
>+                            this.setPlayButtonState(true);

I was a little wary of using |true| here instead of |this.video.paused|, but after some reflection I think it's fine:

1) autoplay videos already appear as paused until we trigger playback; so this will be a noop on the first load [setupInitialState() already called it with true]
2) I don't think you can actually get the media in a .paused == false state at this point
3) even if you did, the worst that would happen would be showing the wrong icon for a moment, because the timeupdate handler will correct the state.

So, r+. :)
Attachment #458255 - Flags: review?(dolske) → review+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: