1975032 - Implement Constructor For RTCEncodedVideoFrame and RTCEncodedAudioFrame

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, enhancement)

Description

[Dan Baker[:dbaker]]

We need to implement Constructor for RTCEncodedVideoFrame and RTCEncodedAudioFrame.

Comment 1

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Bug 1975032 - Implement RTCEncodedVideoFrame and RTCEncodedAudioFrame copy constructors. r?bwc

Comment 2

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Bug 1975032 - Improve code reuse, and implement metadata argument correctly. r?bwc

Comment 3

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Bug 1975032 - Use move semantics on RTCEncodedVideoFrameData and RTCEncodedAudioFrameData. r?bwc

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/da6e8cb19129
https://hg.mozilla.org/integration/autoland/rev/11dbef1dd286
Implement RTCEncodedVideoFrame and RTCEncodedAudioFrame copy constructors. r=bwc,webidl,smaug

Comment 5

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/11dbef1dd286

Comment 6

[Hamish Willee]

FF145 MDN documentation for this can be tracked in https://github.com/mdn/content/issues/41508

I'm trying to work out what metadata we actually support and in what versions for audio and video for RTCEncodedAudioFrame.getMetadata() and the new constructor.

1. Is set of supported metadata the same for the getMetadata and the new constructors?
2. For Video, do we support all the properties in the RTCEncodedVideoFrame.webidl?
   • My assumption is that they are all supported in the same version.
   • The structure is different than spec IDL and it seems we omit rtpTimestamp, receiveTime, captureTime, senderCaptureTimeOffset, mimeType
3. Similarly do we support all the metadata properties in RTCEncodedAudioFrame.webidl

• The IDL doesn't quite match the spec, but we pass the audioLevel WPT test, so wanted to check. Assumption is we still don't support it.

Thanks

Comment 7

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Hamish Willee from comment #6)

1. Is set of supported metadata the same for the getMetadata and the new constructors?

Yes. Tests for that are coming in bug 1993997.

2. For Video, do we support all the properties in the RTCEncodedVideoFrame.webidl?

All except timestamp which is outdated and unpopulated. WPT tests were unfortunately lacking, so I plan to improve them in bug 1993997, but for now you can use https://jsfiddle.net/jib1/5t1mehn4/ to verify support. In Firefox it yields the following for video:

main {
  "contributingSources": [],
  "dependencies": [],
  "frameId": 1,
  "height": 240,
  "payloadType": 120,
  "spatialIndex": 0,
  "synchronizationSource": 4251100087,
  "temporalIndex": 0,
  "width": 320
}

• My assumption is that they are all supported in the same version.

frame.getMetadata() was added in bug 1631263, copy constructor in this bug.

• The structure is different than spec IDL and it seems we omit rtpTimestamp, receiveTime, captureTime, senderCaptureTimeOffset, mimeType

Yes we have at least bug 1944338 to track this.

3. Similarly do we support all the metadata properties in RTCEncodedAudioFrame.webidl

We don't appear to populate sequenceNumber which is surprising. Byron, is this expected? The fiddle shows:

main {
  "contributingSources": [],
  "payloadType": 109,
  "synchronizationSource": 721873839
}

• The IDL doesn't quite match the spec, but we pass the audioLevel WPT test, so wanted to check. Assumption is we still don't support it.

Good catch. That's a false positive since undefined < 0 || undefined > 1 is always false:

      if (metadata.audioLevel < 0 || metadata.audioLevel > 1) {
        self.postMessage("Invalid audioLevel value");
        return;
      }

I'll fix it in bug 1993997.

Comment 8

[Byron Campen [:bwc]]

We aren't populating the sequence number because we aren't being supplied with one by libwebrtc:

https://searchfox.org/firefox-main/source/third_party/libwebrtc/audio/channel_send_frame_transformer_delegate.cc#217

I'm not sure why that is.

Comment 9

[Hamish Willee]

(In reply to Byron Campen [:bwc] from comment #8)

We aren't populating the sequence number because we aren't being supplied with one by libwebrtc:

The docs do have the comment "and no sequenceNumber because this is an outgoing frame.", - also in spec "The RTP sequence number as defined in [RFC3550]. Only exists for incoming audio frames."

The ordinary tests also show receiveTime on Chrome which doesn't show up in the fiddle (by which I mean I ran the wpt tests and stopped them at a point I could inspect chrome results.)

I'd love to verify that with a test if you have one.

Comment 10

[Hamish Willee]

EDITED FYI PS. Looking at the spec https://w3c.github.io/webrtc-encoded-transform/#dom-rtcencodedaudioframe-constructor it doesn't sound like it requires you to specify all members - it reads like they get cloned from the original doc and then any options that are specified overwrite those values.

Just letting you know that this theory works on Firefox but gave me errors on Chrome.

To test this I modified https://jsfiddle.net/jib1/5t1mehn4/ to create a new frame:

  const newFrame = new RTCEncodedVideoFrame(frame, {metadata: { height: "260" }} )

FAIL video test: InvalidModificationError: Failed to construct 'RTCEncodedVideoFrame': Cannot create a new VideoFrame: new metadata has member(s) missing. failed

Similarly, if I got metadata (so a full object) and modified the width metadata then created a frame with that. Chrome gave me this value.

FAIL video test: InvalidModificationError: Failed to construct 'RTCEncodedVideoFrame': Cannot create a new VideoFrame: invalid modification of RTCEncodedVideoFrameMetadata.

Comment 11

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Hi Hamish, thanks for testing! You are correct. We try to follow spec. We've added tests for this in https://github.com/web-platform-tests/wpt/pull/55588 and yes they are failing in Chrome.