1188887 - Setting currentTime when readyState is HAVE_NOTHING incorrectly throws an exception: Invalid State

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jean-Yves Avenard [:jya]]

Attempting to set the currentTime value when readyState yield:
NS_ERROR_DOM_INVALID_STATE_ERR
(code: http://mxr.mozilla.org/mozilla-central/source/dom/html/HTMLMediaElement.cpp#1470)

This appears to be an incorrect behaviour:

From the spec:
https://html.spec.whatwg.org/multipage/embedded-content.html#dom-media-currenttime

"if the media element's readyState is HAVE_NOTHING, then it must set the media element's default playback start position to the new value; otherwise, it must set the official playback position to the new value and then seek to the new value. The new value must be interpreted as being in seconds."

So it must set the default playback start position which is:
https://html.spec.whatwg.org/multipage/embedded-content.html#default-playback-start-position

Media elements also have a default playback start position, which must initially be set to zero seconds. This time is used to allow the element to be seeked even before the media is loaded.

A side note: We have no code whatsoever to handle a start time that isn't 0.

Comment 1

[JW Wang [:jwwang] [:jw_wang]]

So we need to allow seek anyway no matter it can be fulfilled immediately or be done after loading some resources.

Comment 2

[Jean-Yves Avenard [:jya]]

Yes.

The "default playback start position" is just a placeholder containing the future seek position for when we actually seek.

While readyState is HAVE_NOTHING, querying the currentTime attribute will return the default playback start position.
As soon as the seek has started, default playback start position is set to 0 and won't ever be used again

(that's how I read the spec at least)

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Jean-Yves Avenard [:jya] from comment #2)
> As soon as the seek has started, default playback start position is set to 0.
This statement is interesting. I wonder what its purpose is.

They have:
4.8.14.5 Loading the media resource
9. Let the media element's default playback start position be zero.
10. Let the initial playback position be zero.

Point 9 corresponds to your statement and point 10 introduces a new term "initial playback position" which is never explained.

Comment 4

[Jean-Yves Avenard [:jya]]

It seems to be related to a media controller, which we don't implement anyway. Can probably ignore for now.

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: Do not throw exception if readyState is HAVE_NOTHING.

Instead we abort the seek as per spec.

Comment 6

[Jean-Yves Avenard [:jya]]

Hmmm... I see two different algorithms described when setting currentTime. One is as per describe in the description of this bug:
https://html.spec.whatwg.org/multipage/embedded-content.html#dom-media-currenttime

"The currentTime attribute must, on getting, return the media element's default playback start position, unless that is zero, in which case it must return the element's official playback position. The returned value must be expressed in seconds. On setting, if the media element has a current media controller, then the user agent must throw an InvalidStateError exception; otherwise, if the media element's readyState is HAVE_NOTHING, then it must set the media element's default playback start position to the new value; otherwise, it must set the official playback position to the new value and then seek to the new value. The new value must be interpreted as being in seconds."

the other is to simply start seeking, this depends on the version of the spec. regardless, it's not as simply as I first thought

Comment 7

[Philip Jägenstedt [:foolip]]

This was a spec bug: https://github.com/whatwg/html/pull/286

Comment 8

[Jean-Yves Avenard [:jya]]

Attachment: P1. Allow seeking when readyState is HAVE_NOTHING.

Comment 9

[Jean-Yves Avenard [:jya]]

Attachment: P2. Add mochitest testing new seek behavior.

Comment 10

[Jean-Yves Avenard [:jya]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=92dfb85b4dbf

Comment 11

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8678769 [details] [diff] [review]
P1. Allow seeking when readyState is HAVE_NOTHING.

Review of attachment 8678769 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/html/HTMLMediaElement.cpp
@@ +1513,5 @@
>      aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
>      return;
>    }
>  
>    if (mReadyState == nsIDOMHTMLMediaElement::HAVE_NOTHING) {

This should move above the check of |if (!mDecoder)| since mDecoder is null before setting the 'src' attribute.

And you need a new test case to test the non-MSE case.

Comment 12

[Jean-Yves Avenard [:jya]]

(In reply to JW Wang [:jwwang] from comment #11)
> Comment on attachment 8678769 [details] [diff] [review]
> P1. Allow seeking when readyState is HAVE_NOTHING.
> 
> Review of attachment 8678769 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/html/HTMLMediaElement.cpp
> @@ +1513,5 @@
> >      aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
> >      return;
> >    }
> >  
> >    if (mReadyState == nsIDOMHTMLMediaElement::HAVE_NOTHING) {
> 
> This should move above the check of |if (!mDecoder)| since mDecoder is null
> before setting the 'src' attribute.
> 
> And you need a new test case to test the non-MSE case.

This is something else though.
the test for !mDecoder is for handling the case where no media resource is yet attached to the media element and is something clarified in https://github.com/whatwg/html/pull/286 ;

I will submit a part 3 patch to handle this and remove that test all together (so we fall back into the readyState == HAVE_NOTHING)

Comment 13

[Jean-Yves Avenard [:jya]]

(In reply to JW Wang [:jwwang] from comment #11)

> This should move above the check of |if (!mDecoder)| since mDecoder is null
> before setting the 'src' attribute.
> 
> And you need a new test case to test the non-MSE case.

I don't see how we could test this (readyState = HAVE_NOTHING) for plain media in a way that wouldn't be intermittently failing

We can however test for w3c bug 286 (where no src is set before seeking) reliably. That will be part 4

Comment 14

[JW Wang [:jwwang] [:jw_wang]]

This confuses me. Before setting 'src', how do we know whether this element will run MSE or not?

Comment 15

[Philip Jägenstedt [:foolip]]

(In reply to Jean-Yves Avenard [:jya] from comment #12)
> (In reply to JW Wang [:jwwang] from comment #11)
> > Comment on attachment 8678769 [details] [diff] [review]
> > P1. Allow seeking when readyState is HAVE_NOTHING.
> > 
> > Review of attachment 8678769 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/html/HTMLMediaElement.cpp
> > @@ +1513,5 @@
> > >      aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
> > >      return;
> > >    }
> > >  
> > >    if (mReadyState == nsIDOMHTMLMediaElement::HAVE_NOTHING) {
> > 
> > This should move above the check of |if (!mDecoder)| since mDecoder is null
> > before setting the 'src' attribute.
> > 
> > And you need a new test case to test the non-MSE case.
> 
> This is something else though.
> the test for !mDecoder is for handling the case where no media resource is
> yet attached to the media element and is something clarified in
> https://github.com/whatwg/html/pull/286 ;

I didn't intend for that spec change to correspond to any change in implementations, it was just the description of the readyState HAVE_METADATA that was wrong, but the actual requirements are all elsewhere, in the definitions of attributes, methods, internal algorithms, etc.

Per the spec as it now is, the only way that setting currentTime can throw an exception is if there's a media controller, but Gecko doesn't implement that. In other words, if any of the code paths that throw INVALID_STATE_ERR are reachable, then it's not per spec. If you think that an exception is the only reasonable thing to do in these situations, please file spec bugs :)

Comment 16

[Jean-Yves Avenard [:jya]]

(In reply to Philip Jägenstedt from comment #15)
> Per the spec as it now is, the only way that setting currentTime can throw
> an exception is if there's a media controller, but Gecko doesn't implement
> that. In other words, if any of the code paths that throw INVALID_STATE_ERR
> are reachable, then it's not per spec. If you think that an exception is the
> only reasonable thing to do in these situations, please file spec bugs :)

We are throwing an exception when the media element is attached to a Media Stream, I don't see where this case is handled in the spec.

Otherwise, the main reason we were throwing is that no-one ended up updated it since 2011 when the specs were changed ! :(

Comment 17

[Philip Jägenstedt [:foolip]]

For things that aren't seekable at all, like streams, then the seekable attribute should have no ranges, in which case the seek algorithm will eventually do nothing at all, per "If there are no ranges given in the seekable attribute then set the seeking IDL attribute to false and abort these steps."

Comment 18

[Jean-Yves Avenard [:jya]]

Attachment: P3. Never throw an exception when seeking.

Per spec, no exception can ever be thrown when seeking.
Only leaving cases indicating a failure in one of the component (which other than a second to usec conversion overflow will never happen anyway).

The MediaStream case is as per comment 17.

Comment 19

[Jean-Yves Avenard [:jya]]

Attachment: P4. Add mochitest to test new behavior.

Comment 20

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8679394 [details] [diff] [review]
P4. Add mochitest to test new behavior.

Review of attachment 8679394 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/test/test_seek_nosrc.html
@@ +19,5 @@
> +
> +var v = document.createElement('video');
> +document.body.appendChild(v);
> +SimpleTest.registerCleanupFunction(function () {
> +  v.parentNode.removeChild(v);

You can just say |v.remove()|.

Comment 21

[Jean-Yves Avenard [:jya]]

(In reply to JW Wang [:jwwang] from comment #20)
> You can just say |v.remove()|.

I copied that from the other seek mochitest :)

Comment 22

[Jean-Yves Avenard [:jya]]

Don't forget P1 :)

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/faca58991660
https://hg.mozilla.org/integration/mozilla-inbound/rev/0edf6192ddd1
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c27972e56a7
https://hg.mozilla.org/integration/mozilla-inbound/rev/39af5c53fad6

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/86d1b3a8aecb

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/faca58991660
https://hg.mozilla.org/mozilla-central/rev/0edf6192ddd1
https://hg.mozilla.org/mozilla-central/rev/3c27972e56a7
https://hg.mozilla.org/mozilla-central/rev/39af5c53fad6
https://hg.mozilla.org/mozilla-central/rev/86d1b3a8aecb