WICG / video-rvfc

video.requestVideoFrameCallback() incubation
https://wicg.github.io/video-rvfc/
Other
54 stars 19 forks source link

Should animation-frame callback IDs be shared between video elements? #25

Closed smfr closed 4 years ago

smfr commented 4 years ago

From the spec it sounds like each HTMLVideoElement has its own set of callback identifiers. That would make it hard for a client that tracks multiple video elements to keep track of which identifier to cancel.

I would suggest making the callback identifiers document-wide.

tguilbert-google commented 4 years ago

Yes, having the identifiers document wide sounds good. At least this way they can be added into a map.

I would keep the list of callbacks per element. Otherwise, I think it would be odd to be able to cancel a callback from a different element than the one that registered it.

foolip commented 4 years ago

This is also great material for tests.

tguilbert-google commented 4 years ago

Currently, the spec says the callback identifiers should start at 0. If you detach a video from its parent and add it to an iframe, you could potentially get a collision with callback IDs.

Maybe current pending callbacks should be invalidated when the element is detached?

tguilbert-google commented 4 years ago

The answer to the original question is "yes". The spec has been updated, so I am closing this issue out

Maybe current pending callbacks should be invalidated when the element is detached?

This will be done at the same time the onemptied event.

mounirlamouri commented 4 years ago

To clarify, the emptied event is not a replacement for detachment as there is no requirement in the spec, as far as I can remember, for the element to be emptied when changing frames. I believe Chrome does that but AFAICT it's a implementation limitation coming from WebMediaPlayer that we should be able to work around. Because here the problem is really the callback ids, would it make sense to add some language to make sure the callback ids are cleared when changing document?

mounirlamouri commented 4 years ago

On second thought, I wonder if we could deal with this by having an object instead of an integer to be used as callback ID.

Something like:

const vFCbId = video.requestVideoFrameCallback(() => {});
video.cancelVideoFrameCallback(vFCbId);

It looks exactly the same as using an int but the int can't be read and we can link back to the video element for simplicity:

vFCbId.video.cancelVideoFrameCallback(vFCbId);

Alternatively, we could have:

vFCbId.cancel();

While still keeping vFCbId.video for reference.

WDYT?

tguilbert-google commented 4 years ago

I like the idea of a dedicated structure, but I think it goes against how similar callback interfaces operate. For consistency with requestAnimationFrame and requestIdleCallback, I would maintain the use of just a handle.

As I mentioned offline, a concern with the vFCbId.video is keeping video element alive longer than they need to.

I would lean toward updating the spec to invalidate callbacks on Document change. Is there an event already spec'ed out that describes attaching an element to a new Document?

Yay295 commented 4 years ago

I could see using a Symbol instead of an integer, but I also agree that keeping it similar to the existing functions is a good idea too.

tguilbert-google commented 4 years ago

TIL about Symbol.

After all, I do think that keeping requestVideoFrameCallback() aligned with requestAnimationFrame() and requestIdleCallback() is the best course of action.

I added language to the spec about clearing the callbacks when changing documents.

mounirlamouri commented 4 years ago

requestAnimationFrame() and requestIdleCallback() are document-specific APIs while this is a video-specific API. We can move a video across documents, it seems unfortunate in principle to have this limited because the "similar functions" are document-based.

dalecurtis commented 4 years ago

I don't think we should pursue something like that unless there's prior art. Even though the name has changed, we're still technically trying to keep the concepts the same as rAF.

mounirlamouri commented 4 years ago

After discussing this further, it seems that we are trying to resolve a non-problem. Having the ids being bound to the HTMLVideoElement may be fine as is given that the API is available at the HTMLVideoElement. It wouldn't be surprising for the end user for the returned id to be specific to that element. It may indeed require some users to save that handle with the associated video but the two alternatives we have (document-scoped id or using a different pattern) have other issues.

In general, it seems that cancelling a rAF on a video element is a pretty small use cases. These callbacks run very often and need to be requested every time. It's good to offer to developers the ability to cancel a subscription but it's very likely [virtually] no one will do that. We should probably not over-optimise this given that it's working.

I would recommend to close this.

tguilbert-google commented 4 years ago

Thanks Mounir, I agree with your assessment, and will be rolling back #52 and a portion of #36.