Bug 169841 - Add libwebrtc backend support for RTCRtpSender::replaceTrack
Summary: Add libwebrtc backend support for RTCRtpSender::replaceTrack
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on:
Blocks: 169662
  Show dependency treegraph
 
Reported: 2017-03-17 17:36 PDT by youenn fablet
Modified: 2017-03-24 11:01 PDT (History)
5 users (show)

See Also:


Attachments
Patch (10.56 KB, patch)
2017-03-17 17:38 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (24.33 KB, patch)
2017-03-22 12:57 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (31.49 KB, patch)
2017-03-23 13:28 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2017-03-17 17:36:57 PDT
replaceTrack might be handy in various cases.
Comment 1 youenn fablet 2017-03-17 17:38:36 PDT
Created attachment 304840 [details]
Patch
Comment 2 youenn fablet 2017-03-22 12:57:25 PDT
Created attachment 305119 [details]
Patch
Comment 3 Alex Christensen 2017-03-22 15:39:01 PDT
Comment on attachment 305119 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=305119&action=review

> Source/WebCore/Modules/mediastream/RTCRtpSender.idl:42
> -    [MayThrowException] Promise<void> replaceTrack(MediaStreamTrack withTrack);
> +    [MayThrowException] Promise<void> replaceTrack(MediaStreamTrack? withTrack);

Why are we deviating from the spec here?
Comment 4 youenn fablet 2017-03-22 15:43:58 PDT
(In reply to Alex Christensen from comment #3)
> Comment on attachment 305119 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=305119&action=review
> 
> > Source/WebCore/Modules/mediastream/RTCRtpSender.idl:42
> > -    [MayThrowException] Promise<void> replaceTrack(MediaStreamTrack withTrack);
> > +    [MayThrowException] Promise<void> replaceTrack(MediaStreamTrack? withTrack);
> 
> Why are we deviating from the spec here?

Probably a typo, I need to file an issue.
Description at https://www.w3.org/TR/webrtc/#dom-rtcrtpsender-replacetrack expects withTrack to be nullable.
Comment 5 Jon Lee 2017-03-22 20:17:51 PDT
Comment on attachment 305119 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=305119&action=review

> Source/WebCore/Modules/mediastream/RTCRtpSender.cpp:69
>  {

Are we missing step 4 of replaceTrack?

> Source/WebCore/Modules/mediastream/RTCRtpSender.cpp:75
> +    if (!withTrack) {

shouldn't this be "if (withTrack)"?

> Source/WebCore/Modules/mediastream/RTCRtpSender.cpp:85
>          return Exception { TypeError };

Should this be promise.reject(TypeError) instead?
Comment 6 youenn fablet 2017-03-23 09:25:22 PDT
(In reply to Jon Lee from comment #5)
> Comment on attachment 305119 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=305119&action=review
> 
> > Source/WebCore/Modules/mediastream/RTCRtpSender.cpp:69
> >  {
> 
> Are we missing step 4 of replaceTrack?

The implementation order is not matching the spec order.
It is true that it might cause subtle differences between implementation.

> 
> > Source/WebCore/Modules/mediastream/RTCRtpSender.cpp:75
> > +    if (!withTrack) {
> 
> shouldn't this be "if (withTrack)"?

Again, we are not matching the spec step by step.
It seems to me the spec should branch early on when withTrack is null.
That would be more readable and easier to implement anyway.

> 
> > Source/WebCore/Modules/mediastream/RTCRtpSender.cpp:85
> >          return Exception { TypeError };
> 
> Should this be promise.reject(TypeError) instead?

Oh, that is a left over when we were catching exceptions and rejecting promises.
replaceTrack should not have MayThrowException anymore.
Comment 7 youenn fablet 2017-03-23 13:28:16 PDT
Created attachment 305221 [details]
Patch
Comment 8 WebKit Commit Bot 2017-03-24 11:01:21 PDT
Comment on attachment 305221 [details]
Patch

Clearing flags on attachment: 305221

Committed r214357: <http://trac.webkit.org/changeset/214357>
Comment 9 WebKit Commit Bot 2017-03-24 11:01:26 PDT
All reviewed patches have been landed.  Closing bug.