Bug 169919 - [WebRTC] Fix final mid of a transceiver
Summary: [WebRTC] Fix final mid of a transceiver
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebRTC (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-03-21 08:47 PDT by Florent Defay
Modified: 2018-03-12 10:12 PDT (History)
5 users (show)

See Also:


Attachments
Patch (1.82 KB, patch)
2017-03-21 08:55 PDT, Florent Defay
alex: review-
florent.defay: commit-queue?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Florent Defay 2017-03-21 08:47:11 PDT
When we receive an offer, we set final mid of a transceiver with the provisional mid. This causes transceiver to use audio source with video payload or the contrary when the offer media description mids dont match the provisional mids.

We should set the final mid of a transceiver with the mid of the media description which has the same type as the transceiver.
Comment 1 Florent Defay 2017-03-21 08:55:06 PDT
Created attachment 305006 [details]
Patch
Comment 2 Alejandro G. Castro 2017-03-22 02:31:50 PDT
Comment on attachment 305006 [details]
Patch

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

Thanks for the patch, can you explain how we can reproduce this issue?

> Source/WebCore/ChangeLog:3
> +        [WebRTC] Fix final mid of a transceiver

Try to be more descriptive explaining the issue with the title, instead of explaining how you fix it, such as: Problem when setting final mid for the ...

> Source/WebCore/ChangeLog:9
> +        (WebCore::MediaEndpointPeerConnection::setLocalDescriptionTask):

Add here the comments about the changes you did.

> Source/WebCore/Modules/mediastream/MediaEndpointPeerConnection.cpp:374
>              RTCRtpTransceiver* transceiver = matchTransceiver(transceivers, [&mediaDescription] (RTCRtpTransceiver& current) {
> -                return current.provisionalMid() == mediaDescription.mid;
> +                return current.mid().isNull() && current.sender().trackKind() == mediaDescription.type;
>              });
>              if (transceiver)
> -                transceiver->setMid(transceiver->provisionalMid());
> +                transceiver->setMid(mediaDescription.mid);

I don't understand the solution, the matchTransceiver function makes sure the provisionalMid and the mediaDescription mid are the same value, and the transceiver exist, so it should be safe to assign the provisionalMid in that situation. Also your change of the matchTransceiver function is not matching correctly because there could be multiple tracks with the same type.

I think if the payload of a mediaDescription does not match and their mids match the problem should be somewhere else.
Comment 3 Florent Defay 2017-03-23 02:33:37 PDT
(In reply to Alejandro G. Castro from comment #2)
> Thanks for the patch, can you explain how we can reproduce this issue?
This is a random behaviour. We get this using a basic page establishing WebRTC connection audio and video between two set-top boxes. Symmetric setup.
The WebRTC backend is openwebrtc.

> > Source/WebCore/ChangeLog:3
> > +        [WebRTC] Fix final mid of a transceiver
> 
> Try to be more descriptive explaining the issue with the title, instead of
> explaining how you fix it, such as: Problem when setting final mid for the
> ...
OK. What about:
Problem with transceiver assigned a mid not matching the payload in the mediaDescription

What happens is that we have audio on mid1 and video on mid2 and a transceiver is set with mid1 but with payload of video.

> I think if the payload of a mediaDescription does not match and their mids
> match the problem should be somewhere else.
OK.
Comment 4 Radar WebKit Bug Importer 2017-08-29 09:48:45 PDT
<rdar://problem/34134363>
Comment 5 youenn fablet 2018-03-12 10:12:39 PDT
Closing this one since this patch is changing an endpoint which was removed from trunk.