ennuicastr / libavjs-webcodecs-polyfill

A polyfill for the WebCodecs API. No, really.
82 stars 8 forks source link

Allow overloaded constructor call `new VideoFrame(image)` #30

Closed rostyq closed 1 month ago

rostyq commented 1 month ago

According to MDN Docs, the VideoFrame constructor can be called with one argument.

https://developer.mozilla.org/en-US/docs/Web/API/VideoFrame/VideoFrame

Yahweasel commented 1 month ago

This does not follow the WebCodecs spec.

According to the WebCodecs spec, init is only optional for an HTMLVideoElement or another VideoFrame, as those are the only cases where there's another source for the timestamp. The fact that the VideoFrame constructor doesn't work with another VideoFrame as input is a major blunder, but unfortunately not one that this PR fixes. The lack of support for init-optionality with HTMLVideoElement is pure laziness, but the behavior of this PR in that case is not correct. If you would like to make a version of this patch that correctly imports the timestamp in those cases and throws in all other cases, that would be appreciated, but until then, the current behavior is closer to the spec than this PR is.

rostyq commented 1 month ago

I see. Thank you for pointing out. I use this one for reading from HTMLVideoElement, so I am interested in implementing it as close as possible to specs. At least I've encountered an exception, "If image’s networkState attribute is NETWORK_EMPTY", which is not covered by this polyfill.

I am thinking to define the constructor signature as follows:

export class VideoFrame {
    constructor(data: HTMLVideoElement | VideoFrame, init?: VideoFrameInit)
    constructor(data: CanvasImageSource, init: VideoFrameInit)
    constructor(data: BufferSource, init: VideoFrameBufferInit)
    constructor(data: CanvasImageSource | BufferSource | VideoFrame,
        init?: VideoFrameInit | VideoFrameBufferInit) { /* ... */ }

    private _constructCanvas(image: any, init: VideoFrameInit) { /* ... */ }
    /* ... */
}

The signature of _constructCanvas remains original to this polyfill, only special cases for HTMLVideoElement and VideoFrame are handled in the constructor method. I suppose for the VideoFrame case, I can reuse the _constructBuffer method for initializing a new VideoFrame.

What do you think?

Yahweasel commented 1 month ago

I certainly won't demand that you write any code that isn't useful to you ;). Although I wouldn't mind you writing the constructor for VideoFrame-from-VideoFrame, don't be bothered to if you don't need it. The major complication of that case is that it should support using a browser VideoFrame or a polyfill VideoFrame. 'course, it currently supports neither ^^´

That looks like the right signature to me. But, I wouldn't want to fix the signature if it doesn't correctly support HTMLVideoElement anyway, which it currently doesn't. I believe all that should be necessary for a correct implementation of HTMLVideoElement is the network status as you suggest, plus using the timestamp out of the the video element itself. If you update the PR to have this signature and fix HTMLVideoElement, I'd be happy to accept that.

Yahweasel commented 1 month ago

Thanks!