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

Finalize API Name discussion #44

Closed
tguilbert-google opened this issue Mar 27, 2020 · 39 comments
Closed

Finalize API Name discussion #44

tguilbert-google opened this issue Mar 27, 2020 · 39 comments

Comments

@tguilbert-google
Copy link
Member

@tguilbert-google tguilbert-google commented Mar 27, 2020

There is an ongoing discussion on the name of the API. This issue aims to move the discussion into the official repo.

See also: TAG design review, Mozilla standards position thread, and webkit-dev thread.

Picking up where the Mozilla position thread left off:

@padenot:

  • window.requestAnimationFrame tells you that you need to do something in the function you passed, because it wants the next frame of animation. It's effectively "requesting an animation frame", this is pull: the request comes from something, and information flows from the author's code to the implementation (in this case, a series of drawing command)
  • HTMLVideoElement.requestAnimationFrame informs you that a new video frame is now available and is going to be composited, telling you a bunch of metrics. Incidentally, a drawImage can be done there as well. Nothing is "requesting an animation frame", this is what window.requestAnimationFrame is for.

I'd prefer if the name of this new feature would convey what it does, but I can live with the current name (we're discussing other naming issues on the HTMLVideoElement.requestAnimationFrame repo) if we don't find something better.

My POV is that video.rAF does "request an animation frame", because it's forcing the "update the rendering" steps to happen. Granted, the steps we request might not be the very next available steps like with window.rAF.

I think that the semantics of video.requestAnimationFrame are similar enough to window.requestAnimationFrame that the consistency in naming outweigh the benefits that come from a different name.

From MDN:

The window.requestAnimationFrame() method tells the browser that you wish to perform an animation and requests that the browser calls a specified function to update an animation before the next repaint.

I think one could say:

The video.requestAnimationFrame() method tells the browser that you wish to perform an animation if a new video frame was presented and requests that the browser calls a specified function to update an animation before the next repaint.

I don't know if the "if a new video frame was presented" is what makes the APIs different enough from each other that a new name is warranted. Is that distinction what differentiates the push and pull APIs? (since the information flow is "schedule -> wait for event -> run" instead of "schedule -> run")
In both cases, these are async APIs asking for the opportunity to animate at the next tick some conceptual clock: a v-sync clock for window.rAF and a video clock for video.rAF.

onFrameAvailable() was brought up in the webkit thread (but this was before video.rAF ran in the rendering steps, so sorry for the potential strawperson argument). My objection to that name is that I would expect that it would be called for every frame. If there was a 120fps video on a 60hz screen, I would expect onFrameAvailable() to fire at 120fps, but this API would only be called at 60hz.

There is also onFramePresented() and onFramePresentation(). I'm not sure what other proposals there are (onComposition()? A name that doesn't have the on prefix?). What are the gains in clarity from these names? Would there still be a time parameter in the callback signature for these new API names?

@tguilbert-google tguilbert-google changed the title Finalize API Name discussion Finalize video-raf API Name discussion Mar 27, 2020
@tguilbert-google tguilbert-google changed the title Finalize video-raf API Name discussion Finalize API Name discussion Mar 27, 2020
@smfr
Copy link

@smfr smfr commented Mar 27, 2020

What if you turn things around:

window.requestAnimationFrame(callback, video)

@smfr
Copy link

@smfr smfr commented Mar 27, 2020

You could imagine this as a generic "give me a rAF when this thing changes": window.requestAnimationFrame(callback, canvas), window.requestAnimationFrame(callback, image) (for animated image).

@dalecurtis
Copy link
Collaborator

@dalecurtis dalecurtis commented Mar 27, 2020

That's interesting, but seems inconsistent with the direction the animation folks have chosen to go. I.e., they seem to have standardized on AnimationFrameProvider mix-in instead of overloading:
https://html.spec.whatwg.org/multipage/imagebitmap-and-animations.html#animation-frames

@smfr
Copy link

@smfr smfr commented Mar 27, 2020

But that's the source of all this confusion.

@dalecurtis
Copy link
Collaborator

@dalecurtis dalecurtis commented Mar 27, 2020

Can you be more specific in the context of your suggestion and my reply? I.e., what are your views on consistency of naming where there exist shipped standards? Are you indicating that you just dislike the AnimationFrameProvider mix-in concept generally? Just in this context?

whatwg/html#3587 has more details on the change that standardized the AnimationFrameProvider mix-in.

@smfr
Copy link

@smfr smfr commented Mar 27, 2020

Is anyone shipping AnimationFrameProvider other than Chrome? It doesn't seem too late to change it.

@dalecurtis
Copy link
Collaborator

@dalecurtis dalecurtis commented Mar 27, 2020

Per the I2S Firefox has support behind a flag; I'm not sure if they shipped it though. It has been shipped for over 2 years now though. Your proposal wouldn't work for their use case since workers don't have a window object though.

@othermaciej
Copy link

@othermaciej othermaciej commented Mar 27, 2020

Regular rAF is supposed to be called before the screen update. Calling it after a video frame is presented seems inconsistent with that. (Or is it supposed to be after the frame is ready but before it actually hits the screen? Which may not be practically implementable...)

@dalecurtis
Copy link
Collaborator

@dalecurtis dalecurtis commented Mar 27, 2020

See #37 this is called before the frame hits the screen in a best effort manner. Specifically 43732bc

@othermaciej
Copy link

@othermaciej othermaciej commented Mar 27, 2020

OK, maybe I read too much into the past tense in the phrase "if a new video frame was presented".

@dalecurtis
Copy link
Collaborator

@dalecurtis dalecurtis commented Apr 2, 2020

To keep the conversation going: Is Safari's position currently, "We are okay with the name, but don't like that it's hung off the video element and would prefer a worker/window scope?"

In that vein, @smfr can you elaborate on that proposal? Read another way, you're not proposing that we abandon the AnimationFrameProvider mix-in concept, but instead proposing that AFP grow element-specific overrides. As such both workers and normal contexts will benefit.

I do have some followup questions:

  1. What do you expect animated image callbacks to do? Per the canvas spec a canvas must always render the default frame of an animated image, so it's unclear what a page would do with this callback.

  2. What would a callback from the canvas do? Are their any automated processes by which content will arrive in a canvas that was not explicitly triggered by the page?

As is, video seems like the only case which would benefit from our proposed API - so it seems premature to make it globally available via Window/Worker contexts.

@padenot, do you have any naming suggestions from the Firefox side?

@dalecurtis
Copy link
Collaborator

@dalecurtis dalecurtis commented Apr 2, 2020

Oh and one third question:

  • Do you have any concerns about breaking existing pages by adding new parameters to a long-established API?

@smfr
Copy link

@smfr smfr commented Apr 2, 2020

Safari's position is that the name, as a function on video element, is confusing. We don't consider AnimationFrameProvider to have had sufficient cross-vendor discussion, so we think that the API design is still flexible.

We think that an extension to the window.requestAnimationFrame() API, where an element being observed is passed in, should be given serious consideration, and best matches the intended behavior for this API. I don't foresee a problem with adding new parameters to window.requestAnimationFrame, but maybe they exist.

If we have to keep this as a function on the video element, we strongly suggest a name change.

@dalecurtis
Copy link
Collaborator

@dalecurtis dalecurtis commented Apr 2, 2020

Thanks for elaborating. We will consult with the animation and window.* OWNERS in Chrome.

@dalecurtis
Copy link
Collaborator

@dalecurtis dalecurtis commented Apr 2, 2020

@smfr to continue pursuing all avenues: If we change names, what are your proposals? Are you also proposing we drop the attachment to running as a rendering step with the name change?

@smfr
Copy link

@smfr smfr commented Apr 2, 2020

@smfr to continue pursuing all avenues: If we change names, what are your proposals? Are you also proposing we drop the attachment to running as a rendering step with the name change?

This question implies that you're not sure how the API should work yet? I think that needs to get nailed down before we bikeshed the naming.

@dalecurtis
Copy link
Collaborator

@dalecurtis dalecurtis commented Apr 2, 2020

No, I'm exploring your objections to how we think the API should work.

@smfr
Copy link

@smfr smfr commented Apr 2, 2020

I think the current wording in https://wicg.github.io/video-raf/#video-raf is close to reasonable (run the callbacks as part of the "update the rendering" steps). However, it doesn't clarify what should happen if the video framerate is faster than the screen refresh framerate, and I think that's essential to figuring out what this API means.

@dalecurtis
Copy link
Collaborator

@dalecurtis dalecurtis commented Apr 2, 2020

Filed #46. It's implicit in the API shape, but it should be explicit. Thanks for raising.

In such cases, callbacks will only be fired for composited frames. E.g., for a 120fps video on a 60Hz display, we'd only render every other frame and only issue callbacks for those rendered.

@fserb
Copy link
Member

@fserb fserb commented Apr 2, 2020

I find overloading window.RAF to include video a bit weird.
I understand that video.RAF would have a slightly different meaning, but I think consistency beats precision here. I think getting behind the AnimationFrameProvider mix-in sounds very reasonable too, even if we have to adjust the definition expectation of it.

@othermaciej
Copy link

@othermaciej othermaciej commented Apr 2, 2020

The AnimationFrameProvider mix-in is only used for global scopes in the HTML spec. IT seems intended as a factoring tool to share some common functionality between Window and worker global scopes. Stretching that to also apply to elements seems a little odd.

I also see a proposed use for XRSession, but that is a little bit different, since it seems to more tightly sync to the frame rate than the proposal for video here, and seems intended for things that would update the XR display.

Are there other uses of it besides these?

@othermaciej
Copy link

@othermaciej othermaciej commented Apr 2, 2020

XRSession is an interesting case, it raises the question of what happens when you want to stay in sync with a video being rendered into XR. Do you do video.rAF or XRSession.rAF? The answer to this might clarify whether Simon's suggestion to have an optional element passed into window.rAF makes sense.

@dalecurtis
Copy link
Collaborator

@dalecurtis dalecurtis commented Apr 2, 2020

You'd likely want to queue a XRSession.rAF once you receive the video.rAF; since I'm guessing window.rAF isn't running at the same rate as XRSession.rAF.

@othermaciej
Copy link

@othermaciej othermaciej commented Apr 3, 2020

Given that, wouldn't it be better if XRSession.rAF took an optional video parameter, so you don't need to double rAF? Besides being awkward, the double rAF may incur extra latency, and would gratuitously run the update the rendering steps for the page, when what was really wanted was just an XRSession update.

i.e. this makes Simon's proposal sound comparatively better, in light of current uses of AnimationFrameProvider. This would update the rendering steps for a given provider after a frame for a video is ready. The video itself is not really a provider, since it is generally not possible to paint anything other than the video on the video's screen update cycle.

@dalecurtis
Copy link
Collaborator

@dalecurtis dalecurtis commented Apr 3, 2020

After some more thought I don't think either my suggestion or yours works well in this case. The XR display is entirely decoupled from the main display's cadence and needs the video rendered at a different rate than the main display or playback will be jerky. E.g., 24Hz in 60hz vs 90Hz XR.

For this use case, a site would be better off using an off-screen video and webgl_video_texture updating at the XR display rate via XRSession.rAF. In Chrome, this will cause the video to render at the XR display rate and not the main display rate -- resulting in smooth playback on the XR display.

An earlier version of the explainer noted that webgl_video_texture plans to adopt the same VideoFrameMetadata structure we end up defining for video.rAF exactly for use cases like this:

partial interface WebGLVideoTexture {
    // Allows DOM-less WebGL usage where texImage2D() is the only compositor to
    // avoid the need for requestAnimationFrame() to get video frame metadata.
    VideoFrameMetadata VideoElementTargetVideoTexture(GLenum target, HTMLVideoElement video)
}

Beyond the playback smoothness issue, I worry that the alignment of frame updates will cause too many missed XR deadlines to be useful. In Chrome's implementation, much of our ability to actually deliver the frame callbacks ahead of the window.rAF is due to the rendering steps running at the same rate and slightly after video composition. 

Backing up a bit:

  • #44 (comment): Informally, of those OWNERS who replied, they prefer video.rAF over extending window.rAF. If window.rAF needed to be extended, they'd prefer something like window.requestVideoAnimationFrame.

  • #44 (comment): If we were to change the name of video.rAF, what does Safari propose? video.requestVideoFrame()?

@smfr
Copy link

@smfr smfr commented Apr 3, 2020

video.requestVideoFrame

It's not a request for a video frame! It's a request for an "update the rendering" when a frame is available. I just don't think there's a good name with this API surface.

Maybe video.requestAnimationFrameForVideoFrame but that's so long-winded.

Your OWNERS may not like it, but I'd like to hear from other browser vendors.

@othermaciej
Copy link

@othermaciej othermaciej commented Apr 3, 2020

@dalecurtis thanks for the thoughtful analysis of this case. Also thanks for bringing up video metadata. Because of that, I noticed for the first time that in the spec, video.requestAnimationFrame has a callback with a different signature than window.requestAnimationFrame. This suggests that it can never adopt the AnimationFrameProvider mixin, and that likely the method should be named different.

@smfr I think video.requestVideoFrame is a reasonable approximation to the long-winded name. Given the VideoFrameMetadata parameter, it actually is telling the callback about a video frame. It just has the side effect of also triggering the "update the rendering" steps

(Who are the best Mozilla folks to weigh in?)

@dalecurtis
Copy link
Collaborator

@dalecurtis dalecurtis commented Apr 3, 2020

@padenot is the contact from the Mozilla side we've been working with.

We're still partial to the requestAnimationFrame() name since it runs as part of the rendering steps, but if that's a hard blocker for others we're happy with whatever name the group agrees upon.

@othermaciej
Copy link

@othermaciej othermaciej commented Apr 3, 2020

I think using the same name with a different signature is not ideal so -1 to video.rAF (but not -1 million).

I personally like requestVideoFrame, because it waits for a video frame to be ready and provides you with metadata about it. It's ok that the name doesn't explicitly convey that it will trigger the "update the rendering" steps. I suspect many authors don't know what that means exactly, so will not be thrown by this.

I think the earlier names starting with on are not good because they go against DOM conventions for what on means. It's generally an event listener attribute, not a method you call.

I think Simon may still like the extra parameter to window.rAF.

Would be good to get other view points.

@padenot
Copy link

@padenot padenot commented Apr 6, 2020

My point of view on the matter is in the very first message of this issue. I agree with various folks here that the proposed name conveys the wrong thing.

requestVideoFrame is problematic because it conveys the fact that the main thread now control the display of video frames, and we don't want that.

The point in my first comment still stand: this is not a request, and this is not an animation. There is however a frame. Consistency is not useful if it ends up being misleading.

We want a name that expresses the fact that a video frame is now queued for composition and will be displayed on the same frame as other things that have been done during this update the rendering. Ideally it would also conveys the fact that the frame can be blited somewhere else via webgl or canvas, since it's already ready for display.

@dalecurtis
Copy link
Collaborator

@dalecurtis dalecurtis commented Apr 6, 2020

Thanks Paul. I've solicited feedback on naming internally and externally (video-dev slack); summary:

  • Support for keeping it as video.rAF.
  • Several suggestions for onXXX names; Google TAG folks concur with @othermaciej that those are verboten.
  • Strong disagreement that "request" as a prefix is misleading. See window.requestIdleCallback for precedent.

Most popular alternative suggestions are of the "requestVideoFrame*" variety.

  • requestVideoFrame
  • requestVideoFrameCallback
  • requestVideoFrameMetadata

Other suggestions:

  • addFrame{State,Notification}Callback
  • animateOnFrame.
  • requestNextFrame
  • setNextFrameCallback.
  • setFrameNotificationCallback.

I suggest we choose one of the rVF names. I'm partial to requestVideoFrameCallback.

@tguilbert-google
Copy link
Member Author

@tguilbert-google tguilbert-google commented Apr 6, 2020

I am also on board with requestVideoFrameCallback.

@padenot
Copy link

@padenot padenot commented Apr 8, 2020

requestVideoFrameCallback makes sense, because we're actually requesting a callback to be called on a video frame (implied: compositing event, but I can live with this, it's not like I'm proposing anything, and I don't have better idea).

The fact that request is a prefix is not the problem, is what seem to be requested that need to be in line with what the API does. requestIdleCallback makes sense to me, it does what it says.

I agree with you both that requestVideoFrameCallback is a good name for this, thanks for reaching out to everybody!

@dalecurtis
Copy link
Collaborator

@dalecurtis dalecurtis commented Apr 8, 2020

Thanks Paul. @othermaciej @smfr is requestVideoFrameCallback okay to Safari?

(I sent an e-mail to Edge folks the other day to see if they wanted to weigh in as well)

@smfr
Copy link

@smfr smfr commented Apr 8, 2020

We can live with it.

@Yay295
Copy link

@Yay295 Yay295 commented Apr 8, 2020

Is that video.requestVideoFrameCallback or window.requestVideoFrameCallback? Would "Video" be too redundant in the first case (i.e. video.requestFrameCallback)?

@dalecurtis
Copy link
Collaborator

@dalecurtis dalecurtis commented Apr 8, 2020

Thanks @smfr! Thomas is putting together a change to update the spec and will close this issue out once that lands.

@Yay295 I considered that, but in light of the commentary from others and the fact that a <video> may also have audio it seemed reasonable to explicitly specify which portion of the media you're getting a callback for.

@tguilbert-google
Copy link
Member Author

@tguilbert-google tguilbert-google commented Apr 10, 2020

The spec has been updated, the rename in Chromium should land by the end of the day, and I will rename this repo to video-rvfc.

Seeing as we have an acceptable name, I am closing this issue.

Thank you everyone for the discussion!

@sushraja-msft
Copy link

@sushraja-msft sushraja-msft commented Apr 10, 2020

Apologies for the late response. My vote would go for requestVideoFrameCallback as well.

The window.requestIdleCallback for precedence puts to rest my worry about this not being a "request". video.requestVideoFrameCallback rather than window.requestVideoFrameCallback sits well with me as good separation of concerns.

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

No branches or pull requests

8 participants