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

Invalid Web IDL for VideoFrame: attributes cannot accept dictionary types #278

Closed
tidoust opened this issue Jun 12, 2021 · 14 comments · Fixed by #299
Closed

Invalid Web IDL for VideoFrame: attributes cannot accept dictionary types #278

tidoust opened this issue Jun 12, 2021 · 14 comments · Fixed by #299
Labels
breaking Interface changes that would break current usage (producing errors or undesired behavior).

Comments

@tidoust
Copy link
Member

tidoust commented Jun 12, 2021

#208 introduced attributes codedRect and visibleRect to VideoFrame with a VideoFrameRect dictionary type.

Web IDL forbids the use of a dictionary type for attributes, see the Dictionaries section: "Dictionaries must not be used as the type of an attribute or constant". For the rationale, see for example whatwg/webidl#938.

I believe the alternative is to define an explicit getter operation.

@dalecurtis
Copy link
Contributor

@sandersdan @chcunningham

@sandersdan
Copy link
Contributor

sandersdan commented Jun 14, 2021

Assuming that we use a getter, should we be freezing the result so that we can return the same dictionary multiple times?

@chcunningham
Copy link
Collaborator

I find whatwg/webidl#938 surprising. I follow that it may not be obvious that a dictionary attribute is always copied, but I'm confused why returning it via getter method helps clarify? @domenic

+1 to Dan's question above.

@chcunningham chcunningham added the breaking Interface changes that would break current usage (producing errors or undesired behavior). label Jun 17, 2021
@domenic
Copy link

domenic commented Jun 17, 2021

I follow that it may not be obvious that a dictionary attribute is always copied, but I'm confused why returning it via getter method helps clarify?

It is very surprising to JavaScript developers when foo.bar !== foo.bar (which is the case for dictionary attributes).

It is not surprising to JavaScript developers when foo.bar() !== foo.bar().

This is codified in https://w3ctag.github.io/design-principles/#attributes-like-data . So using a method instead of a getter avoids this problem.

@sandersdan
Copy link
Contributor

@domenic It's still not clear whether a freezing these dictionary attributes would be sufficient to meet the requirements?

@domenic
Copy link

domenic commented Jun 17, 2021

Freezing dictionaries isn't really a pattern we've seen on the web platform, apart from a couple of legacy cases. You'd have to work outside of the system of IDL bindings, i.e. just use object or any for your type and then manually call the Web IDL conversions spec to create a JS object from your dictionary, and the ECMAScript spec to freeze the resulting JS object. And you'd have to pick a particular time to do this.

A getter method is probably better. Or, just inline the attributes into the parent interface, e.g. instead of foo.bar.baz or foo.bar().baz just do foo.barBaz.

@domenic
Copy link

domenic commented Jun 17, 2021

Looking into the cause here, I think the problem is that you're duplicating a lot of work people have already done by creating the DOMRectReadOnly and DOMRectInit types... I'd suggest using those instead. Note that DOMRectReadOnly is an interface so it can be the type of an attribute.

You'd have to do your own normalization on the input DOMRectInit to round any values and outlaw infinites/NaNs, but that's fine.

@sandersdan
Copy link
Contributor

sandersdan commented Jun 17, 2021

We considered DOMRect but ultimately rejected it because it wasn't using integers and that was important to the meaning of rectangles in WebCodecs (eg. we require rects to be exactly divisible by the subsampling resolution, and we use EnforceRange). This is much more important where rectangles are parameters than where they are attributes, although I wouldn't want to use two types.

We also previously did have inlined attributes (eg. cropLeft), but our APIs also accept these structures as parameters so it is very useful to be able to easily reference them (eg. frame.allocationSize({ rect: frame.codedRect })).

I think we prefer methods over switching to DOMRect, but some outside perspective could be helpful here.

@domenic
Copy link

domenic commented Jun 17, 2021

Note that the integer vs. double types are not visible to JavaScript developers or other API users. (JavaScript only has doubles.) They are only manifested in how many "automatic" normalization steps you get done by Web IDL automatically. So DOMRectInit + a manual normalization step to integers on inputs has the exact same semantics as an integer-only dictionary.

And DOMRectReadOnly on the output side has the exact same semantics as an integer-only dictionary, as long as you only put integers into it in your spec. (Well, aside from DOMRectReadOnly being an interface, and thus being valid Web IDL to use for this.)

So I'd really suggest DOMRectReadOnly + DOMRectInit.

@sandersdan
Copy link
Contributor

types are not visible to JavaScript developers or other API users

This is surprising to me, I was expecting future direct WASM bindings to be more IDL-like?

So DOMRectInit + a manual normalization

I'm assuming you are proposing that our parameters should be (DOMRect or DOMRectReadOnly or DOMRectInit)?

@domenic
Copy link

domenic commented Jun 17, 2021

They should be DOMRectInit, since a DOMRect or DOMRectReadOnly automatically satisfies the contract of DOMRectInit.

@domenic
Copy link

domenic commented Jun 17, 2021

To give a bit more detail (sorry, I was typing the above while in a meeting):

  • codedRect and visibleRect in VideoFrame would become DOMRectReadOnly

    • Bonus: developers get bottom and right values computed for them, not just the height and width values. Plus x and y as synonyms for left and top. This matches the rest of the platform in how rectangles are exposed, which is good.
    • You need to update their getter steps to either lazily assemble a DOMRectReadOnly, returning the same one each time, or you need to create a DOMRectReadOnly ahead of time and return it.
  • visibleRect in VideoPlaneInit would become DOMRectInit

    • This has the impact of requiring developers to pass objects containing { x, y, width, height } properties instead of { left, top, width, height }. This is good because it's more congruent with the rest of the platform.
    • Developers can pass any DOMRect or DOMRectReadOnly here, including values from codedRect and visibleRect, and its x/y/width/height values will get used.
    • You would add a step to "To check if a VideoFramePlaneInit is a valid VideoFramePlaneInit" which checks that x/y/width/height exist, and are finite and non-NaN, so that it throws if not.
    • You would update step 9 of the new VideoFrame() constructor to clamp the values to integers before assigning to [[visible width]] and [[visible height]]. The easiest way to do this might be to convert the values to an unsigned long long.
      • It looks like the x/y (currently left/top) properties are ignored by that algorithm? So maybe the switch to x/y doesn't matter...
  • Similarly for rect in VideoFrameCopyToOptions

    • You'd need to update "Parse CopyTo Rect"

@chcunningham
Copy link
Collaborator

@domenic

You would update step 9 of the new VideoFrame() constructor to clamp the values to integers before assigning to [[visible width]] and [[visible height]]. The easiest way to do this might be to convert the values to an unsigned long long.

Is it preferable to convert vs require? We could instead check that integers were provided and throw exceptions.

@domenic
Copy link

domenic commented Jun 17, 2021

I can't think of any other API on the web platform which throws an exception on non-integer values (including your own specification of VideoFrameRect), so I think convert would be better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Interface changes that would break current usage (producing errors or undesired behavior).
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants