Bug 169841

Summary:

Add libwebrtc backend support for RTCRtpSender::replaceTrack

Product:

WebKit

Reporter:

youenn fablet <youennf>

Component:

WebCore Misc.

Assignee:

youenn fablet <youennf>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

achristensen, commit-queue, eric.carlson, jer.noble, jonlee

Priority:

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

169662

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none

youenn fablet

Reported 2017-03-17 17:36:57 PDT

replaceTrack might be handy in various cases.

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
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

youenn fablet

Comment 1 2017-03-17 17:38:36 PDT

Created attachment 304840 [details] Patch

youenn fablet

Comment 2 2017-03-22 12:57:25 PDT

Created attachment 305119 [details] Patch

Alex Christensen

Comment 3 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?

youenn fablet

Comment 4 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.

Jon Lee

Comment 5 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?

youenn fablet

Comment 6 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.

youenn fablet

Comment 7 2017-03-23 13:28:16 PDT

Created attachment 305221 [details] Patch

WebKit Commit Bot

Comment 8 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>

WebKit Commit Bot

Comment 9 2017-03-24 11:01:26 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.