w3c / media-source

Media Source Extensions
https://w3c.github.io/media-source/
Other
268 stars 58 forks source link

Should fetch algorithm failure trigger detaching from a media element? #18

Closed wolenetz closed 8 years ago

wolenetz commented 8 years ago

Migrated from w3c bugzilla tracker. For history prior to migration, please see: https://www.w3.org/Bugs/Public/show_bug.cgi?id=27854

It was previously assigned to Adrian Bateman. Editors will sync soon to determine who to take this bug.

jdsmith3000 commented 8 years ago

The resource selection algorithm will ultimately set the networkState to NETWORK_EMPTY if the media element has neither a src attribute or a source element child. This seems like a sufficient trigger for the MSE algorithms.

foolip commented 8 years ago

Yes, but another possibility is that there is a src attribute or source element child, in which cast networkState will not reach NETWORK_EMPTY. Instead, the media element will be playing a new resource, while the old MediaSource object is still attached. (That this doesn't happen in implementations is because they don't match the spec.)

wolenetz commented 8 years ago

Thanks @foolip (see also #17). We'll likely have the MSE spec define an algorithm that the HTML spec calls, so that in short term we don't block MSE on HTML spec change (both can and should change).

jdsmith3000 commented 8 years ago

Same comment here as for issue #17. Can the existing "Detaching from a media element" algorithm be made applicable to clarifying that it is also initiated when networkState reaches NETWORK_NO_SOURCE?

foolip commented 8 years ago

Tying it to networkState reaching NETWORK_NO_SOURCE is not ideal, because that isn't the "reset hook" that other things in HTML uses (the media element load algorithm is) and that's also not how it's implemented in at least Blink.

(Note that networkState is set to NETWORK_NO_SOURCE as the first step of the resource selection algorithm, but the HTML spec is written such that things are already reset when you reach that point.)

jdsmith3000 commented 8 years ago

Both Issues #17 and #18 should resolve with pull request #56.

foolip commented 8 years ago

I think that this issue should remain open, because there isn't yet a call in HTML to the detach steps for the error cases, only for the load algorithm (#17).

wolenetz commented 8 years ago

Re-opening per https://github.com/w3c/media-source/issues/18#issuecomment-211395205. @jdsmith3000 please take a look - can this be resolved similarly by non-normative note(s) in MSE? Are additional call-out points in the informative HTML51 reference needed (and if so, that informative reference snapshot URL in MSE spec will also need updating).

jdsmith3000 commented 8 years ago

@foolip @Wolenetz: I wasn't expecting MSE to explicitly point at every place where the detach algorithm was called. Is that required?

The current non-normative note is pretty general, and doesn't explicitly point at the existing call on load(). Note the "may be called" wording.

Going forward, this algorithm is intended to be externally called and run in any case where the attached MediaSource, if any, must be detached from the media element. It may be called on HTMLMediaElement [HTML51] operations like load() and fetch() in addition to, or in place of, when the media element transitions to NETWORK_EMPTY.

foolip commented 8 years ago

I could add the calls to HTML (Living Standard) and I'd be satisfied with that if everyone else is. The question is where exactly those calls should be. I can see exactly where it happens in Blink, but could someone describe where the hooks are in Edge? How about Gecko? I assume it's not entirely interoperable, so this is a chance to fix that.

jdsmith3000 commented 8 years ago

@foolip Are you asking what behaviors we support in Edge? It's currently the legacy "transition to NETWORK_EMPTY". We can't make any changes to call detach on load() or fetch() errors for a bit.

foolip commented 8 years ago

OK, so it sounds like it's not currently interoperable, and one would have to do a bit of in-depth testing to figure out what might be required for compat, and also think a bit about what's actually a good idea for web developers.

jdsmith3000 commented 8 years ago

I have viewed these changes as evolutionary, where the legacy behavior would overlap with the HTML5.1 driven one. Is there an argument for making this transition uniformly across browsers?

paulbrucecotton commented 8 years ago

@foolip and @jdsmith3000: It seems like we have agreed that no more changes are needed to HTML 5.1. Can we re-close this issue with no more changes to MSE?

foolip commented 8 years ago

We currently don't have interoperability on this point, and I think those working on the MSE spec are in the best position to get to the bottom of this, but I've now filed https://github.com/whatwg/html/issues/1098 to track it where change itself, if any, will likely be made.

jdsmith3000 commented 8 years ago

@foolip Do I understand correctly that WHATWG has made changes for fetch failures and you want the Media TF to coordinate making the same changes to HTML5.1? If so, please highlight the HTML5.1 change that is needed.

For now, I've left issue #17 & #18 together in the MSE change record, but will link to the commit only for issue #17 for now.

paulbrucecotton commented 8 years ago

If so, please highlight the HTML5.1 change that is needed.

I might be wrong here and @foolip can probably give a better answer but since https://github.com/whatwg/html/issues/1098 is still open without any responses or suggested conclusions it is NOT yet obvious how HTML might be changed here.

foolip commented 8 years ago

Right, no changes have been made to HTML (any copy) to fix this. Something has to be done, though, because Blink does detach in https://html.spec.whatwg.org/multipage/embedded-content.html#dedicated-media-source-failure-steps

foolip commented 8 years ago

Does any implementor have any opinion of whether it's a good idea to detach in the "dedicated media source failure steps", as Blink does?

jdsmith3000 commented 8 years ago

@foolip I'm seeking comment here. In looking at the failure steps as they exist in http://www.w3.org/TR/html51/semantics-embedded-content.html#dedicated-media-source-failure-steps, I don't see much precedent for detaching the source, though track data appears to be removed. Might that be sufficient?

jdsmith3000 commented 8 years ago

@foolip Assuming no further input, I'm inclined to close this issue without making a change. Please comment if you disagree.

paulbrucecotton commented 8 years ago

@foolip

We URGENTLY need to know your opinion whether we can close this issue. We have marked this issue with the V1 Milestone and we are trying to complete V1 issues by June 9. See MSE timeline.

If we do not hear back from you by Tue May 24 then we will close this issue with no change.

/paulc HME WG Chair

foolip commented 8 years ago

Sorry, I was between jobs and not reading standards mail.

There is an existing interoperability problem here that needs to be resolved in some way or another. If you close this there's still https://github.com/whatwg/html/issues/1098 but I think it makes more sense for those working on the MSE spec to look into this. Maybe just move to v2?

I don't have any special insight here, I've merely noticed that Blink detaches any attached media source between steps 6 and 7 of the dedicated media source failure steps, and no spec says so. @wolenetz, @chcunningham, can you look into the history of this and maybe say if it's a good idea or not?

wolenetz commented 8 years ago

I agree we need to detach MediaSource on fetch algorithm failure. The dedicated media source failure steps are not invoked for fetch failure of source children, just object and attribute modes, so inserting detachment into dedicated media source failure steps would be incomplete. Further, the fetch algorithm can (in some failure modes) abort the resource selection algorithm, when we'd want to detach too, in steps:

If the connection is interrupted after some media data has been received, causing the user agent to give up trying to fetch the resource If the media data is corrupted If the media data fetching process is aborted by the user

We also need to detach on the following fetch algorithm failures which don't abort the resource selection algorithm:

If the media data cannot be fetched at all, due to network errors, causing the user agent to give up trying to fetch the resource 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

But, we don't want to detach the MediaSource in the following resource fetch algorithm step:

Final step: If the user agent ever reaches this step (which can only happen if the entire resource gets loaded and kept available): abort the overall resource selection algorithm.

wolenetz commented 8 years ago

A minimal change to the MSE non-normative detachment note, which is currently:

Going forward, this algorithm is intended to be externally called and run in any case where the attached MediaSource, if any, must be detached from the media element. It may be called on HTMLMediaElement [HTML51] operations like load() and fetch() in addition to, or in place of, when the media element transitions to NETWORK_EMPTY.

Might be (this seems good enough for @jdsmith3000 and myself to be the content of a PR):

Going forward, this algorithm is intended to be externally called and run in any case where the attached MediaSource, if any, must be detached from the media element. It may be called on HTMLMediaElement [HTML51] operations like load() and resource fetch algorithm failures in addition to, or in place of, when the media element transitions to NETWORK_EMPTY. Resource fetch algorithm failures are those which abort either the resource fetch algorithm or the resource selection algorithm, with the exception that the "Final step" [HTML51] is not considered a failure that triggers detachment.

foolip commented 8 years ago

@wolenetz, can you elaborate a bit about the reasons for detaching on failure? What bad things would happen if that line of code were removed from Blink?

Would it make sense to spec exactly what Blink does (detach in the dedicated media source failure steps) or is the ideal something slightly different?

wolenetz commented 8 years ago

@foolip The HTMLMediaElement spec is currently unclear especially in the resource fetch algorithm failure case for iterating among mode=children in the resource selection algorithm: in such case, the dedicated media source failure steps are not executed. So even if we added MSE detachment as part of the dedicated media source failure steps to the whatwg or w3c html5 spec, then compliant implementations might not do such detachment upon decode error within MSE of a child. Unlike attribute or object mode fetches, this would leave the defunct MediaSource with SourceBuffers attached (since the detachment steps were not run for it). Whether or not Blink handles this spec ambiguity "correctly" is separate from getting the spec ambiguity cleared up. The MSE editors believe (IIUC) that the non-normative change suggested in https://github.com/w3c/media-source/issues/18#issuecomment-221942039 to the MSE spec is enough to fix this MSE v1 issue. I have updated https://github.com/whatwg/html/issues/1098 and made a similar request in w3c's html51 (though the latter is on a really tight time-frame).

foolip commented 8 years ago

this would leave the defunct MediaSource with SourceBuffers attached (since the detachment steps were not run for it)

What are the consequences of this, in turn? In the case of source element children, is it that the resource selection algorithm can then proceed to try another resource, while the MediaSource is still attached? Was the detach originally added to avoid some crash, or to avoid some confusion for web developers?

What I'm thinking is that maybe one could detach only when another resource is loaded, but I'm not clear about all of the reasons for detaching in the first place.

wolenetz commented 8 years ago

@foolip, one consequence is yes, in the case of source element children, the resource selection algorithm could proceed to try another resource without detaching the previously attached MediaSource. I'm not aware of a crash, but certainly the point of the non-normative note is to help avoid confusion for web developers and better interop in the interim while HTML51 is getting better clarification around MSE detach. Leaving it non-normative helps give flexibility to HTML51 to decide best when to actually do the detach: should it occur adjacent to "forget the media resource specific tracks", rather than be gated on some networkState transition that's not in all the paths?

wolenetz commented 8 years ago

For now, I'll land the non-normative change. @foolip, would you like a v2 MSE spec bug around this to adjust the spec relative to whatever HTML eventually includes?

(This comment edited 6/6/16 to remove HTML51 and MSE v1NonBlocking, since both w3c's html51 and MSE v1 are on a tight time-frame.)