Closed
Bug 1331158
Opened 8 years ago
Closed 8 years ago
Renegotiation doesn't actually change the receive codec configuration after 49 update landing
Categories
(Core :: WebRTC, defect, P1)
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)
(deleted),
patch
|
ng
:
review+
|
Details | Diff | Splinter Review |
+++ 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).
Assignee | ||
Updated•8 years ago
|
Rank: 13
status-firefox53:
--- → affected
Assignee | ||
Comment 1•8 years ago
|
||
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)
Assignee | ||
Comment 2•8 years ago
|
||
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)
Assignee | ||
Comment 3•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Version: 45 Branch → 53 Branch
Comment 4•8 years ago
|
||
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 5•8 years ago
|
||
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)
Assignee | ||
Comment 6•8 years ago
|
||
(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()
Assignee | ||
Updated•8 years ago
|
Attachment #8826813 -
Attachment is obsolete: true
Assignee | ||
Comment 7•8 years ago
|
||
CodecsDifferent() rewritten
Attachment #8826902 -
Flags: review?(paulrkerr)
Assignee | ||
Updated•8 years ago
|
Attachment #8826817 -
Attachment is obsolete: true
Attachment #8826817 -
Flags: review?(paulrkerr)
Comment 8•8 years ago
|
||
(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. :)
Assignee | ||
Comment 9•8 years ago
|
||
(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 10•8 years ago
|
||
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+
Comment 11•8 years ago
|
||
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
Comment 12•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Updated•8 years ago
|
status-firefox50:
--- → unaffected
status-firefox51:
--- → unaffected
status-firefox52:
--- → unaffected
You need to log in
before you can comment on or make changes to this bug.
Description
•