Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HTMLVideoElement.requestVideoFrameCallback() #429

Closed
3 of 5 tasks
tguilbert-google opened this issue Oct 9, 2019 · 10 comments
Closed
3 of 5 tasks

HTMLVideoElement.requestVideoFrameCallback() #429

tguilbert-google opened this issue Oct 9, 2019 · 10 comments

Comments

@tguilbert-google
Copy link

tguilbert-google commented Oct 9, 2019

Hello TAG!

I'm requesting a TAG review of:

Further details:

We recommend the explainer to be in Markdown. On top of the usual information expected in the explainer, it is strongly recommended to add:

  • Links to major pieces of multi-stakeholder review or discussion of this specification:
    The WICG Proposal has some public comments.
  • Links to major unresolved issues or opposition with this specification: N/A

You should also know that:

This proposal is still in the early stages.

We'd prefer the TAG provide feedback as (please select one):

  • open issues in our GitHub repo for each point of feedback
  • open a single issue in our GitHub repo for the entire review
  • leave review feedback as a comment in this issue and @-notify [@tguilbert-google]
@dbaron
Copy link
Member

dbaron commented Nov 20, 2019

I think one thing that's worth considering here is whether modeling this after Window.requestAnimationFrame is the right choice. That's a relatively old API that I think isn't looked at all that favorably these days, and there may be better alternatives. That said, it's possible it's the right thing here. (For example, it seems like it might be desirable here to skip frames under high load... and using this API might lead to that happening?)

@tguilbert-google
Copy link
Author

Yes. When reviewing the TAG API Design Principle, I noticed that promises are preferred, but there was some talk of generalizing the AnimationFrameProvider interface mixin, which uses callbacks.

@dbaron
Copy link
Member

dbaron commented Nov 20, 2019

I wonder whether something based on Streams would make sense here. (And to be extra clear, when I say "I wonder", I really mean that I'm not sure.)

@domenic
Copy link
Member

domenic commented Nov 26, 2019

My take is that the requestAnimationFrame design has mostly stood the test of time, apart from the overlong name. (And on that point, consistency/familiarity is more valuable than creating a new shorter name for new APIs that share the same semantics.)

In particular, here are a comparison with other paradigms:

  • Promises are for one-and-done events. Although "the next animation frame" is a one-and-done event, and it could be useful for some cases to provide a nextFrame promise, my impression is that many cases benefit from a "call me every frame" API, for which callback-accepting works well. Emulating this by re-registering for the nextFrame promise, repeatedly, seems a bit roundabout.

  • Streams (or async iterators) are not a good fit, because they represent streaming chunks of data that you are pulling from some source, and for which you never want to miss an occurrence. If you stop pulling from a stream, the data will buffer up inside the stream, until you start pulling again. In contrast, if you stop watching for animation frames, and then start again, you don't want to first have to work your way through a backlog of all the animation frames that happened while you were paused.

  • Events (i.e. EventTarget) seem like a conceptual and semantic fit, but somehow... a bit heavyweight. They do have the right semantics: a broadcast from a central authority; subscribe/unsubscribe at leisure; repeated occurrence; etc. They could definitely work, and perhaps if we were starting from scratch we would consider an "animationframe" event on Window/etc. However, I suspect optimizing away all the extra machinery here would be frustrating for implementers, given that animation frames are frequent, and generally involved in animations or game loops, which need to be fast. Events have bubbling/capturing, Event objects, cancelation, etc. The Event object creation in particular seems tricky.

I'd be interested to hear others' perspectives on an "animationframe" event, and in particular how bad the optimizability concerns are, or if there are other concerns besides optimizability. But in general, even if it is a bit more conceptually parsimonious to use events here, in practice I think promoting certain occurrences like animation frames (requestAnimationFrame), timer intervals (setInterval), and idle callbacks (requestIdleCallback) to their own dedicated callback-accepting functions is OK.

@kenchris
Copy link

kenchris commented Dec 2, 2019

I tend to agree with @domenic here that consistency really matters here, and a callback seems fine as this will be called for every frame

@tguilbert-google
Copy link
Author

Thank you for your comments!

Are there any questions that need further elaboration? As per Domenic's comment, does anyone have any other perspectives on the event based model?

@alice alice removed this from the 2019-12-03-f2f-cupertino milestone Jan 27, 2020
@smfr
Copy link

smfr commented Jan 29, 2020

My feedback from webkit-dev (https://lists.webkit.org/pipermail/webkit-dev/2020-January/031030.html):

First, the name is confusing. It sounds like you're requesting a frame from the video, but it's really a "frame available" callback. Why not call it onFrameAvailable()?

Second, its interaction with normal requestAnimationFrame() and the HTML event loop needs to be better defined. Where in in the https://html.spec.whatwg.org/multipage/webappapis.html#event-loop-processing-model do these callbacks fire?

I would expect something that's called "requestAnimationFrame" to only fire in the "update the rendering" steps; requestAnimationFrame is a "before rendering" callback. So firing a callback with the same name at other times seems like it will lead to author confusion.

The author's expectation should be that any content/style changes they make inside a requestAnimationFrame callback will appear on-screen in the same frame as other changes in the same event loop cycle, and that requestAnimationFrame won't be called more often than is necessary to update the screen at the appropriate frame rate.

@tguilbert-google
Copy link
Author

tguilbert-google commented Mar 3, 2020

Here is the draft specification. It incorporates @smfr's feedback. Notably, the callbacks now fire as part of the rendering steps, immediately before window.requestAnimationFrame() callbacks.

We experimented with using microtask timing instead of animation-frame timing. Painting from a microtask was significantly less smooth for 60fps video on a 60hz screen, due to some vsyncs having double (wasted) paints, and some having none. As such, I don't think a promise or an event with microtask timing makes much sense.

Animation-frame timing is the best option for the intended purpose of this API. requestAnimationFrame() could still be an event or a differently named method. However, on that front, I also tend to agree with @domenic on the consistency of the naming being favorable.

@dbaron dbaron changed the title HTMLVideoElement.requestAnimationFrame() HTMLVideoElement.requestVideoFrameCallback() Apr 10, 2020
@cynthia
Copy link
Member

cynthia commented May 27, 2020

@kenchris and I discussed this during our VF2F.

As for the design itself, we are pretty happy to see this move forward. As for the naming; we don't have strong opinions on what it should be called as long as it is descriptive of what it does and is consistent enough with the rest of the platform.

Thanks for bringing this to our attention, and let us know if you have any significant design changes that warrant a second review.

@cynthia cynthia closed this as completed May 27, 2020
@tguilbert-google
Copy link
Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants