Open Bug 1430255 Opened 6 years ago Updated 2 months ago

RTCRtpContributingSource timestamp has new timebase

Categories

(Core :: WebRTC, enhancement, P3)

enhancement

Tracking

()

People

(Reporter: ng, Assigned: ng)

References

(Depends on 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(2 files, 1 obsolete file)

See: https://github.com/w3c/webrtc-pc/issues/1690

RTCRtpContributingSource timestamp has new timebase and reference clock, "performance.timeOrigin + performance.now()".
No longer blocks: 1429926
Depends on: 1429926
Attachment #8944317 - Flags: review?(mfroman)
Attachment #8944323 - Flags: review?(mfroman)
Attachment #8944325 - Flags: review?(mfroman)
Attachment #8944323 - Flags: review?(mfroman)
Attachment #8944325 - Flags: review?(mfroman)
Comment on attachment 8944317 [details]
Bug 1430255 - P1 - update RTPSources timebase RtpSourceObserver and PeerConnectionImpl;

https://reviewboard.mozilla.org/r/214548/#review220270

::: media/webrtc/signaling/src/peerconnection/TransceiverImpl.h:147
(Diff revision 5)
>    const std::string mPCHandle;
>    RefPtr<JsepTransceiver> mJsepTransceiver;
>    std::string mMid;
>    bool mHaveStartedReceiving;
>    bool mHaveSetupTransport;
> +  const TimeStamp& mTimeBase;

Is this really supposed to be a reference?  For example, mPCHandle is copied.
Attachment #8944317 - Flags: review?(mfroman) → review+
Comment on attachment 8944323 [details]
Bug 1430255 - P2 - update RTPSources timebase, gtests;

https://reviewboard.mozilla.org/r/214550/#review220272

Looks good to me.
Attachment #8944323 - Flags: review?(mfroman) → review+
Comment on attachment 8944325 [details]
Bug 1430255 - P3 - update RTPSources timebase, js PeerConnection, mochitests

https://reviewboard.mozilla.org/r/214554/#review220274

Looks good to me.
Attachment #8944325 - Flags: review?(mfroman) → review+
Assignee: nobody → na-g
Comment on attachment 8944325 [details]
Bug 1430255 - P3 - update RTPSources timebase, js PeerConnection, mochitests

https://reviewboard.mozilla.org/r/214554/#review220336


Static analysis found 1 defect in this patch.
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/media/tests/mochitest/test_peerConnection_audioSynchronizationSources.html:44
(Diff revision 6)
>           `Synchronization source has audio level. (${source.audioLevel})`);
>        ok(source.audioLevel < 128,
>           `Synchronization source audio level < 128. (${source.audioLevel})`);
>        ok(source.timestamp,
>           `Synchronization source has timestamp (${source.timestamp})`);
> -      ok(Number.isInteger(source.timestamp),
> +      ok(window.performance.now() + window.performance.timeOrigin -

Error: 'ok' is not defined. [eslint: no-undef]
Rank: 8
Attachment #8944317 - Flags: review?(jib)
Attachment #8944323 - Flags: review?(jib)
Attachment #8944325 - Flags: review?(jib)
I see you already have r+, so I don't think you need review from me. Lgtm from a brief glance.
Attachment #8944325 - Attachment is obsolete: true
Moving to p3 because no activity for at least 24 weeks.
Priority: P1 → P3
Severity: normal → S3
See Also: → 1878972
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: