[webkit-reviews] review denied: [Bug 169919] [WebRTC] Fix final mid of a transceiver : [Attachment 305006] Patch

[bugzilla-daemon at webkit.org]

Alejandro G. Castro <alex at igalia.com> has denied Florent Defay
<florent.defay at softathome.com>'s request for review:
Bug 169919: [WebRTC] Fix final mid of a transceiver
https://bugs.webkit.org/show_bug.cgi?id=169919

Attachment 305006: Patch

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




--- Comment #2 from Alejandro G. Castro <alex at igalia.com> ---
Comment on attachment 305006
  --> https://bugs.webkit.org/attachment.cgi?id=305006
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.