Closed Bug 1331158 Opened 3 years ago Closed 3 years ago

Renegotiation doesn't actually change the receive codec configuration after 49 update landing

Categories

(Core :: WebRTC, defect, P1)

53 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 --- unaffected
firefox53 --- fixed

People

(Reporter: jesup, Assigned: jesup)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

+++ 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).
Rank: 13
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 ==
Attachment #8826813 - Flags: review?(nfroyd)
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.
Attachment #8826817 - Flags: review?(paulrkerr)
Version: 45 Branch → 53 Branch
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.
Attachment #8826813 - Flags: review?(nfroyd)
(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()
Attachment #8826813 - Attachment is obsolete: true
CodecsDifferent() rewritten
Attachment #8826902 - Flags: review?(paulrkerr)
Attachment #8826817 - Attachment is obsolete: true
Attachment #8826817 - Flags: review?(paulrkerr)
(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.
Attachment #8826902 - Flags: review+
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
https://hg.mozilla.org/mozilla-central/rev/ec08fbdc84b2
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.