Re: Feedback on MediaController

On Tue, 26 Apr 2011 00:25:27 +0200, Ian Hickson <ian@hixie.ch> wrote:

> On Thu, 21 Apr 2011, Philip Jägenstedt wrote:
>>
>> One kind of quite likely error is that people try to draw the videos to  
>> a
>> canvas in the loadedmetadata event handler. Most of the time this will  
>> work
>> because of the very narrow window between HAVE_METADATA and  
>> HAVE_CURRENT_DATA.
>> However, for those elements that are still actually in HAVE_METADATA,  
>> nothing
>> will be painted per the drawImage spec: "if the image argument is an
>> HTMLVideoElement object whose readyState attribute is either  
>> HAVE_NOTHING or
>> HAVE_METADATA, then the implementation must return without drawing  
>> anything."
>> If readyState is racy, then of course such scripts will fail randomly,  
>> but if
>> such failure is rare enough, some sites will inevitably come to depend  
>> on
>> readyState being HAVE_CURRENT_DATA in the loadedmetadata event handler,  
>> which
>> would make HAVE_METADATA and HAVE_CURRENT_DATA completely redundant.
>
> To fix this we'd need to do two things: first, make the changing of
> readyState happen in the task the event fires in (which we'll probably do
> as a result of the bugs you filed), and second, make drawImage() fail if
> readyState isn't HAVE_CURRENT_DATA, even if the data actually is
> available.

Yes, but per the spec, drawImage already fails if readyState <  
HAVE_CURRENT_DATA, regardless of whether or not a frame is actually  
available.

> Also, we'll have to define what happens if the handler for
> loadeddata seeks to something that doesn't have data yet and then calls
> drawImage(). Or seeks to something that doesn't have data yet, spins
> synchronously for just long enough to _get_ data, and then calls
> drawImage(). I guess we could make seeking synchronously reset  
> readyState.
>
> Can you clarify if this is what you mean?

Yes, this is what I think should be specified, in excruciating detail :)  
IMO, it would be best to make seeking synchronously set readyState to  
HAVE_METADATA and currentTime to the new value, so that it's never  
possible to drawImage in a script after seeking, regardless of how long  
one busy-waits.

>> > > I'd very much like to see at least the overall issue of race
>> > > conditions resolved. For these aggregate events on MediaController,
>> > > we would have to make sure that they are fired in the same task as
>> > > the last media element changes its readyState and fires the
>> > > corresponding event.
>> >
>> > Firing it in the same task that the last media element fires the
>> > corresponding event seems reasonable, but I still don't understand why
>> > you think it will be helpful to freeze readyState until the event
>> > fires. Can you explain (maybe again) with a concrete example?
>>
>> These are race conditions that I or people I work with have actually
>> tripped on:
>>
>> * When removing the autoplay attribute in any of the loadstart,
>> loadedmetadata, loadeddata or canplay event handlers, it's racy whether
>> or not the media element will actually play.
>
> I'll handle that as part of bug 11981.
>
>
>> * When registering a loadeddata event handler in the loadedmetadata
>> event handler, it's racy whether or not that event handler will run. The
>> same is true of any other pair of events that depend on readyState or
>> networkState transitions. Having this not be racy would be useful when
>> writing tests, e.g. to catch the first canplaythrough event after the
>> seeked event.
>
> I'll handle this as part of these same bugs.

Thanks.

>> * For preload=metadata, when checking networkState in a progress event
>> handler, it's racy between NETWORK_LOADING and NETWORK_IDLE. This makes
>> it impossible to test if we follow the spec on fast networks.
>
> Is there a bug on this one?
>
> (I don't really understand the problem here.)

This is http://www.w3.org/Bugs/Public/show_bug.cgi?id=12175

The concrete problem here is that in order to test that one does the right  
thing for preload=metadata, one would test for the event order loadstart,  
loadedmetadata, loadeddata, suspend. However, due to raciness between  
decoding and the network, this isn't possible. We've smoothed over this by  
implementing a state machine for lying about networkState and readyState  
in certain conditions. I'll report on this to a list near you soonish.

I'm not convinced about what is the best solution to the problem, but  
generally have tried to make things behave as they would on a slow  
network, for consistency and testability.

>> * When currentTime is set in the loadstart event handler, it's racy
>> whether or not it will actually seek (fails if readyState is
>> HAVE_NOTHING).
>
> Making currentTime's behaviour depend on the exposed readyState rather
> than the actual underlying state sounds like a world of pain, but I guess
> we can do that to a limited extent. Is there a bug on this one?

This specific issue would be solved by  
http://www.w3.org/Bugs/Public/show_bug.cgi?id=12267 as readyState would  
then be HAVE_NOTHING during the loadstart event handler, and seeking would  
always fail.

As for the more general issue of how currentTime is updated, I filed  
http://www.w3.org/Bugs/Public/show_bug.cgi?id=12556

>> * When the loop attribute is present and you pause() in the ended event
>> handler, it's racy if you end up paused at the beginning or the end.
>
> I don't see how to fix that. We can't make the event handler synchronous
> since that would make the playback jerky.
>
> In fact, pause() in general is unavoidably "racy" as far as I can tell.

Indeed, this is hard to smooth over. One could of course make pause()  
always stop at the currentTime that scripts see, but since that would  
cause a slight seek backwards in time I don't think we should.

It would be most helpful if after whatever spec changes are made to fix  
race conditions, any race conditions that are knowingly left in are marked  
with warnings, for whatever that might be worth.

-- 
Philip Jägenstedt
Core Developer
Opera Software

Received on Tuesday, 26 April 2011 09:22:47 UTC