Don't do reconfiguration if recv codec order/preference changes

Don't do reconfiguration if recv codec order/preference changes

Adding 'ReceiveCodecsHaveChanged' method that will determine if codecs
HAVE changed, irrespective of order and preference.

Committed: https://crrev.com/874ca3af5b163e1b3fd8802171e44ee252557842
Cr-Commit-Position: refs/heads/master@{#9748}

[Taylor Brandstetter, 2015-08-12 23:08:29]

deadbeef@webrtc.org changed reviewers:
+ pthatcher@webrtc.org

[Taylor Brandstetter, 2015-08-12 23:08:29]

Part 2 of the video flashing bug fix

[pthatcher1, 2015-08-18 20:24:32]

lgtm, assuming you change the method name.

https://codereview.webrtc.org/1291763003/diff/20001/talk/media/webrtc/webrtcv...
File talk/media/webrtc/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1291763003/diff/20001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvideoengine2.cc:863: return true;
{}s please

https://codereview.webrtc.org/1291763003/diff/20001/talk/media/webrtc/webrtcv...
File talk/media/webrtc/webrtcvideoengine2.h (right):

https://codereview.webrtc.org/1291763003/diff/20001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvideoengine2.h:501: std::vector<VideoCodecSettings>
after);
This should be ReceiveCodecsHaveChanged, since the order/preference does matter
for send codecs.

[Taylor Brandstetter, 2015-08-20 23:07:47]

https://codereview.webrtc.org/1291763003/diff/20001/talk/media/webrtc/webrtcv...
File talk/media/webrtc/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1291763003/diff/20001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvideoengine2.cc:863: return true;
On 2015/08/18 20:24:32, pthatcher1 wrote:
> {}s please

Done.

https://codereview.webrtc.org/1291763003/diff/20001/talk/media/webrtc/webrtcv...
File talk/media/webrtc/webrtcvideoengine2.h (right):

https://codereview.webrtc.org/1291763003/diff/20001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvideoengine2.h:501: std::vector<VideoCodecSettings>
after);
On 2015/08/18 20:24:32, pthatcher1 wrote:
> This should be ReceiveCodecsHaveChanged, since the order/preference does
matter
> for send codecs.

Done.

[Taylor Brandstetter, 2015-08-20 23:11:35]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2015-08-20 23:11:36]

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1291763003/#ps60001 (title:
"missed a line")

[commit-bot: I haz the power, 2015-08-20 23:11:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1291763003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1291763003/60001

[commit-bot: I haz the power, 2015-08-21 00:19:26]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-08-21 00:19:34]

Patchset 4 (id:??) landed as
https://crrev.com/874ca3af5b163e1b3fd8802171e44ee252557842
Cr-Commit-Position: refs/heads/master@{#9748}

[tommi, 2015-08-22 21:44:03]

tommi@webrtc.org changed reviewers:
+ tommi@webrtc.org

[tommi, 2015-08-22 21:44:04]

drive-by while rolling

https://codereview.webrtc.org/1291763003/diff/60001/talk/media/webrtc/webrtcv...
File talk/media/webrtc/webrtcvideoengine2.h (right):

https://codereview.webrtc.org/1291763003/diff/60001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvideoengine2.h:500: static bool
ReceiveCodecsHaveChanged(std::vector<VideoCodecSettings> before,
shouldn't these be passed by const ref instead of by value?

[pthatcher1, 2015-08-24 16:19:12]

https://codereview.webrtc.org/1291763003/diff/60001/talk/media/webrtc/webrtcv...
File talk/media/webrtc/webrtcvideoengine2.h (right):

https://codereview.webrtc.org/1291763003/diff/60001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvideoengine2.h:500: static bool
ReceiveCodecsHaveChanged(std::vector<VideoCodecSettings> before,
On 2015/08/22 21:44:04, tommi (webrtc) wrote:
> shouldn't these be passed by const ref instead of by value?

They get sorted immediately after being passed in, so it needs a copy anyway.

[tommi, 2015-08-24 17:05:37]

Does recv_codecs_ need to remain unsorted?  Maybe it would be ok to pass the
vectors as pointers?  I know this isn't a big issue but since the structures are
pretty big and at least one of them is temporary it seems like a waste to copy
them only to sort and discard the copies.

https://codereview.webrtc.org/1291763003/diff/60001/talk/media/webrtc/webrtcv...
File talk/media/webrtc/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1291763003/diff/60001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvideoengine2.cc:953: recv_codecs_ = supported_codecs;
nit: swap()? :)

[pthatcher1, 2015-08-24 17:38:00]

On 2015/08/24 17:05:37, tommi (webrtc) wrote:
> Does recv_codecs_ need to remain unsorted?  Maybe it would be ok to pass the
> vectors as pointers?  I know this isn't a big issue but since the structures
are
> pretty big and at least one of them is temporary it seems like a waste to copy
> them only to sort and discard the copies.
> 

Big?  The vector likely has < 10 things in it (vp8, vp9, h264, fec, rtx), and
each one is a simple struct.  The only thing that could be big is the codec
parameters for h264, maybe.

And this only happens when the receive codecs are set, which is pretty rare. 

So, I think the optimization would be small one.  But if we did do an
optimization, I think it would be a bigger optimization to pass in vectors of
pointers and sort the pointers instead of the structs.  


>
https://codereview.webrtc.org/1291763003/diff/60001/talk/media/webrtc/webrtcv...
> File talk/media/webrtc/webrtcvideoengine2.cc (right):
> 
>
https://codereview.webrtc.org/1291763003/diff/60001/talk/media/webrtc/webrtcv...
> talk/media/webrtc/webrtcvideoengine2.cc:953: recv_codecs_ = supported_codecs;
> nit: swap()? :)

[tommi, 2015-08-24 17:50:21]

On 2015/08/24 17:38:00, pthatcher1 wrote:
> On 2015/08/24 17:05:37, tommi (webrtc) wrote:
> > Does recv_codecs_ need to remain unsorted?  Maybe it would be ok to pass the
> > vectors as pointers?  I know this isn't a big issue but since the structures
> are
> > pretty big and at least one of them is temporary it seems like a waste to
copy
> > them only to sort and discard the copies.
> > 
> 
> Big?  The vector likely has < 10 things in it (vp8, vp9, h264, fec, rtx), and
> each one is a simple struct.  The only thing that could be big is the codec
> parameters for h264, maybe.
> 
> And this only happens when the receive codecs are set, which is pretty rare. 
> 
> So, I think the optimization would be small one.  But if we did do an
> optimization, I think it would be a bigger optimization to pass in vectors of
> pointers and sort the pointers instead of the structs.  
> 
Yeah that's what I was thinking.  However, you are right of course that these
aren't that big.  I took a closer look and I must have been thinking about the
Codec type.  Nevermind then :)

[Taylor Brandstetter, 2015-08-24 18:04:17]

On 2015/08/24 17:05:37, tommi (webrtc) wrote:
> Does recv_codecs_ need to remain unsorted?  Maybe it would be ok to pass the
> vectors as pointers?  I know this isn't a big issue but since the structures
are
> pretty big and at least one of them is temporary it seems like a waste to copy
> them only to sort and discard the copies.
> 
>
https://codereview.webrtc.org/1291763003/diff/60001/talk/media/webrtc/webrtcv...
> File talk/media/webrtc/webrtcvideoengine2.cc (right):
> 
>
https://codereview.webrtc.org/1291763003/diff/60001/talk/media/webrtc/webrtcv...
> talk/media/webrtc/webrtcvideoengine2.cc:953: recv_codecs_ = supported_codecs;
> nit: swap()? :)

I probably could have done that. But I wasn't sure if there was some value to
storing the codecs in their original order, and thought it may be a premature
optimization to change "ReceiveCodecsHaveChanged" to "CheckIfChangedAndSort". I
definitely agree about the swap() though.