Closed Bug 1419093 Opened 2 years ago Closed 2 years ago

Track RTC RTP source objects interface to dictionary spec change

Categories

(Core :: WebRTC, enhancement, P4)

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: ng, Assigned: ng)

References

Details

Attachments

(3 files)

The WebIDL for contributing and synchronization sources currently specifies them as interfaces.  At TPAC 2017 it was agreed that they would become dictionaries, see https://github.com/w3c/webrtc-pc/issues/1533.  We implemented them as dictionaries in bug 1363667, before a PR was filed. This bug exists as a reminder to make sure that our WebIDL matches after the PR is filed and lands.
RTCRtpSynchronizationSource.audioLevel is no longer nullable, and needs to be computed if the RTP ssrc-audio-level header is not present[0].

[0] https://github.com/w3c/webrtc-pc/pull/1668
Attachment #8937608 - Flags: review?(mfroman)
Attachment #8937609 - Flags: review?(mfroman)
Attachment #8937610 - Flags: review?(mfroman)
Comment on attachment 8937608 [details]
Bug 1419093 - P1 - update rtp source IDLs to spec

https://reviewboard.mozilla.org/r/208270/#review214182

Looks good to me.
Attachment #8937608 - Flags: review?(mfroman) → review+
Comment on attachment 8937609 [details]
Bug 1419093 - P2 - update rtp source impl & unit tests

https://reviewboard.mozilla.org/r/208272/#review214220

r+ w/ nit on commit message

::: commit-message-941e2:2
(Diff revision 1)
> +Bug 1419093 - P2 - update rtp source impl & unit tests
> +

Add more to the commit message explaining that these changes result from making all the attributes 'required' in the webidl?
Attachment #8937609 - Flags: review?(mfroman) → review+
Comment on attachment 8937610 [details]
Bug 1419093 - P3 - update rtp source js & mochi tests

https://reviewboard.mozilla.org/r/208274/#review214222

Looks good to me.
Attachment #8937610 - Flags: review?(mfroman) → review+
There have been additional changes in the spec as of 2018/1/11. A new patch will be incoming shortly.
Still looks fine to me.
WebIDL review information: This patch updates RTCRtpContributingSource and RTCRtpSynchronizationSource to their current definitions in the webrtc-pc spec
All WebIDL are changes in patch 1.

Spec links:
RTCRtpContributingSource: https://rawgit.com/w3c/webrtc-pc/master/webrtc.html#dom-rtcrtpcontributingsource
RTCRtpSynchronizationSource: https://rawgit.com/w3c/webrtc-pc/master/webrtc.html#dom-rtcrtpsynchronizationsource
Comment on attachment 8937608 [details]
Bug 1419093 - P1 - update rtp source IDLs to spec

https://reviewboard.mozilla.org/r/208270/#review219878

::: dom/webidl/RTCRtpSources.webidl:26
(Diff revision 4)
>  enum RTCRtpSourceEntryType {
>      "contributing",
>      "synchronization",
>  };
>  
> -dictionary RTCRtpSourceEntry {
> +dictionary RTCRtpSourceEntry : RTCRtpSynchronizationSource {

Could you perhaps add a comment that this is used internally only
Attachment #8937608 - Flags: review+
Pushed by na-g@nostrum.com:
https://hg.mozilla.org/integration/autoland/rev/d9bca0ad6901
P1 - update rtp source IDLs to spec r=mjf,smaug
https://hg.mozilla.org/integration/autoland/rev/ee60c7ad1483
P2 - update rtp source impl & unit tests r=mjf
https://hg.mozilla.org/integration/autoland/rev/259d2de21779
P3 - update rtp source js & mochi tests r=mjf
https://hg.mozilla.org/mozilla-central/rev/d9bca0ad6901
https://hg.mozilla.org/mozilla-central/rev/ee60c7ad1483
https://hg.mozilla.org/mozilla-central/rev/259d2de21779
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.