+++ This bug was initially created as a clone of Bug #1330091 +++ The rewrite of ConfigureRecvMediaCodec() ends up not actually installing the new codec/settings it calculates, since StartReceiving() checks mRecvStream, and if it's set doesn't instantiate a new stream (and decoders). Also, the KeyFrameRequest type wasn't getting used -- the Call api doesn't support it. There's also no way to say 'none' if the two sides didn't agree on a KeyFrameRequest method (PLI or FIR).

UniquePtr's operator== compares the *pointers*, not the contents, which could only be true if you're comparing A with A. This provides a content comparator, so we can compare nsTArray<UniquePtr<Foo>>'s, though we have to duplicate nsTArray's operator== impl using CompareContent() instead of ==

This now works, and avoids rebuilding the receive stream if *nothing* changed - perhaps it could be less aggressive, but this is safe. also avoids messing up the config on early failures, and fixes the KeyFrameRequest code that wasn't hooked up to anything - we calculated the mode, and then because there was no place to put it, did nothing with it.

Comment on attachment 8826817 [details] [diff] [review] Install new receive codec config for WebRTC if it changed Review of attachment 8826817 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp @@ +1910,5 @@ > + > + // XXX std::equal would be as fast or faster here > + for (uint32_t i = 0; i < len; ++i) { > + if (!CompareContents(a[i], b[i])) { > + return true; It seems better to just write this whole function as: typedef UniquePtr<VideoCodecCondig> ptr; return !std::equal(a.begin(), a.end(), b.begin(), b.end() [](const ptr& x, const ptr& y) { return *x == *y; }); or something like that. The lambda will be nicer when we can use C++14, so we can have |const auto&| parameters. No #ifs, no need to have this CompareContents function, etc. etc. Did trying to use std::equal not work, and thus prompted the XXX comment?

Comment on attachment 8826813 [details] [diff] [review] Add UniquePtr array contents comparison method Review of attachment 8826813 [details] [diff] [review]: ----------------------------------------------------------------- See my comment on the other patch for what I think is a better way to handle this case. I think making the == more explicit at the call site is better than adding another function that hides that behavior.

(In reply to Nathan Froyd [:froydnj] from comment #4) > Comment on attachment 8826817 [details] [diff] [review] > Install new receive codec config for WebRTC if it changed > > Review of attachment 8826817 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp > @@ +1910,5 @@ > > + > > + // XXX std::equal would be as fast or faster here > > + for (uint32_t i = 0; i < len; ++i) { > > + if (!CompareContents(a[i], b[i])) { > > + return true; > > It seems better to just write this whole function as: > > typedef UniquePtr<VideoCodecCondig> ptr; > > return !std::equal(a.begin(), a.end(), b.begin(), b.end() > [](const ptr& x, const ptr& y) { > return *x == *y; > }); c++11 can't use the 4-arg (dual-iterators-with-last) version of std::equal, nor can it use versions with predicate functions, so this doesn't work. > or something like that. The lambda will be nicer when we can use C++14, so > we can have |const auto&| parameters. > > No #ifs, no need to have this CompareContents function, etc. etc. Did > trying to use std::equal not work, and thus prompted the XXX comment? I didn't try it -- I cloned the nsTArray operator==, but replaced the innermost comparison to compare the contents instead of the pointers (which seems to be of little use for UniquePtr....) This comment pointed out to me that I could just replace the innermost bit with !(*a[i] == *b[i]), and drop the comparator in UniquePtr -- though I think it is an issue worth thinking about: operator== is close-to-worthless for UniquePtrs. Also - it might be useful for nsTArray to have a helper template for comparing arrays of pointers to objects (comparing contents of the arrays) -- effectively the function I just (re)wrote for CodecsDifferent()

CodecsDifferent() rewritten

(In reply to Randell Jesup [:jesup] from comment #6) > (In reply to Nathan Froyd [:froydnj] from comment #4) > > ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp > > @@ +1910,5 @@ > > > + > > > + // XXX std::equal would be as fast or faster here > > > + for (uint32_t i = 0; i < len; ++i) { > > > + if (!CompareContents(a[i], b[i])) { > > > + return true; > > > > It seems better to just write this whole function as: > > > > typedef UniquePtr<VideoCodecCondig> ptr; > > > > return !std::equal(a.begin(), a.end(), b.begin(), b.end() > > [](const ptr& x, const ptr& y) { > > return *x == *y; > > }); > > c++11 can't use the 4-arg (dual-iterators-with-last) version of std::equal, > nor can it use versions with predicate functions, so this doesn't work. Ah, that's a good point. But looking at C++11 drafts and cppreference (http://en.cppreference.com/w/cpp/algorithm/equal), it looks like a three-arg-with-predicate is supported, so you'd have to do the unequal length check, but after that you could use std::equal. Whichever way you want to; it's out of my hands at the moment. :)

(In reply to Nathan Froyd [:froydnj] from comment #8) > > > return !std::equal(a.begin(), a.end(), b.begin(), b.end() > > > [](const ptr& x, const ptr& y) { > > > return *x == *y; > > > }); > > > > c++11 can't use the 4-arg (dual-iterators-with-last) version of std::equal, > > nor can it use versions with predicate functions, so this doesn't work. > > Ah, that's a good point. But looking at C++11 drafts and cppreference > (http://en.cppreference.com/w/cpp/algorithm/equal), it looks like a > three-arg-with-predicate is supported, so you'd have to do the unequal > length check, but after that you could use std::equal. Whichever way you > want to; it's out of my hands at the moment. :) std::equals isn't useful for this case without predicate functions (c++14 and up) -- we need to define the comparator function. So the final version is: auto len = a.Length(); if (len != b.Length()) { return true; } // XXX std::equal would work, if we could use it on this - fails for the // same reason as above. c++14 would let us pass a comparator function. for (uint32_t i = 0; i < len; ++i) { if (!(*a[i] == *b[i])) { return true; } } return false;

Comment on attachment 8826902 [details] [diff] [review] Install new receive codec config for WebRTC if it changed Review of attachment 8826902 [details] [diff] [review]: ----------------------------------------------------------------- r+ with nits ::: media/webrtc/signaling/src/media-conduit/CodecConfig.h @@ +107,5 @@ > uint8_t mPacketizationMode; > // TODO: add external negotiated SPS/PPS > > + bool operator==(const VideoCodecConfig& aRhs) const { > + if (mType != aRhs.mType || nit: the 'if' seems superfluous, this could just be a 'return !(...'. ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp @@ +4,5 @@ > > #include "CSFLog.h" > #include "nspr.h" > #include "plstr.h" > +#include "mozilla/UniquePtrExtensions.h" nit: probably should be grouped by include path @@ +1053,5 @@ > } > > // Check for the keyframe request type: PLI is preferred > // over FIR, and FIR is preferred over none. > + // XXX - there is no 'none' option in webrtc.org If you filed a webrtc.org bug, you may want to add a reference here.

Pushed by rjesup@wgate.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ec08fbdc84b2 Install new receive codec config for WebRTC if it changed r=ng