search

w3c / webcodecs

WebCodecs is a flexible web API for encoding and decoding audio and video.
https://w3c.github.io/webcodecs/
Other
975 stars 137 forks source link

Inconsistent use of null #561

Closed sandersdan closed 1 year ago

sandersdan commented 1 year ago

The VideoColorSpaceInit definition does not allow fields to be null, but since the fields are optional they can be set to undefined:

dictionary VideoColorSpaceInit {
  VideoColorPrimaries primaries;
  VideoTransferCharacteristics transfer;
  VideoMatrixCoefficients matrix;
  boolean fullRange;
};

VideoColorSpace, however, has nullable fields:

interface VideoColorSpace {
  readonly attribute VideoColorPrimaries? primaries;
  readonly attribute VideoTransferCharacteristics? transfer;
  readonly attribute VideoMatrixCoefficients? matrix;
  readonly attribute boolean? fullRange;
  ...
};

As a result, missing values can become null, and a VideoColorSpace cannot be reliably used in place of a VideoColorSpaceInit. This same pattern appears in several different places in WebCodecs.

The two fixes I can see are:

  1. Change the interfaces to allow undefined but not null.
  2. Change the dictionaries to allow null.

I'm leaning towards (2) as the least surprising option. Any objections?

dalecurtis commented 1 year ago

2 seems right to me, but @padenot @aboba

padenot commented 1 year ago

Agreed.

sandersdan commented 1 year ago

I dug into this a bit and I'm not sure about (2) any more. This affects only cases where an optional attribute of an interface is read as an Init dictionary; which as it turns out only affects VideoColorSpace/VideoColorSpaceInit. I don't think we've had ergonomics complaints to justify adding support for null everywhere else.

As far as I can tell, VideoColorSpace is using nullable types likely because the Chromium bindings generator doesn't support (VideoColorPrimaries or undefined) syntax (https://crbug.com/1293259). We can work around that by using any as the IDL type in Chromium, as WebGPU is already doing for eg. GpuDeviceLostInfo.

So I'm proposing now to go ahead with option (1), and wait on implementing (2) as a separate potential ergonomics issue.

Edit: the other interfaces with nullable values are EncodedAudioChunk, EncodedVideoChunk (both for duration), VideoFrame (many fields), and ImageTrackList (selectedTrack), all of which seem to be reasonable uses. We could swap some or all of those to using undefined as well, though.

dalecurtis commented 1 year ago

Editor's call: No strong opinion; consistency between interfaces/structures should be prioritized.