Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2831333002:
Allow a received audio codec's payload type to change. (Closed)
|
Created:
3 years, 8 months ago by Taylor Brandstetter Modified:
3 years, 8 months ago CC:
webrtc-reviews_webrtc.org, the sun, tterriberry_mozilla.com Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAllow a received audio codec's payload type to change.
This will create another decoder instance, which isn't ideal, but
that's better than the current behavior where things don't work at all.
We still need to fix the root cause of the linked bug, which is that we
don't remember previous payload type mappings when generating an offer.
This CL also adds a check for what is a real error: when a payload type
that was mapped to one codec is changed to map to a different codec.
And lastly, this CL removes a DCHECK for an assumption that was not
valid: that subsequently applied codec lists will always be supersets of
previous lists.
BUG=webrtc:5847
Review-Url: https://codereview.webrtc.org/2831333002
Cr-Commit-Position: refs/heads/master@{#17897}
Committed: https://chromium.googlesource.com/external/webrtc/+/cb3836773c962c3544bb707c8376e14e64d52f70
Patch Set 1 #
Total comments: 10
Patch Set 2 : Updating a test. #
Total comments: 10
Patch Set 3 : Simplifying code, adding comments. #
Total comments: 8
Patch Set 4 : Changing log message and comment #
Messages
Total messages: 18 (6 generated)
deadbeef@webrtc.org changed reviewers: + solenberg@webrtc.org
PTAL
kwiberg@webrtc.org changed reviewers: + kwiberg@webrtc.org
https://codereview.webrtc.org/2831333002/diff/1/webrtc/api/audio_codecs/audio... File webrtc/api/audio_codecs/audio_format.h (right): https://codereview.webrtc.org/2831333002/diff/1/webrtc/api/audio_codecs/audio... webrtc/api/audio_codecs/audio_format.h:47: bool Matches(const SdpAudioFormat& o) const; Is that a good idea in general? Opus communicates the number of channels in the parameters, for example. https://codereview.webrtc.org/2831333002/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine.cc (left): https://codereview.webrtc.org/2831333002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:1531: }()); Why are you removing this check? I thought we weren't allowed to remove or change existing payload type mappings? https://codereview.webrtc.org/2831333002/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2831333002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:1883: } Hmm... here you disallow changing (but not removing) pt -> codec mappings. But that will still allow a removal immediately followed by reusing the now vacant pt for a new codec, which has the same effect as changing the map.
https://codereview.webrtc.org/2831333002/diff/1/webrtc/api/audio_codecs/audio... File webrtc/api/audio_codecs/audio_format.h (right): https://codereview.webrtc.org/2831333002/diff/1/webrtc/api/audio_codecs/audio... webrtc/api/audio_codecs/audio_format.h:47: bool Matches(const SdpAudioFormat& o) const; On 2017/04/21 12:49:00, kwiberg-webrtc wrote: > Is that a good idea in general? Opus communicates the number of channels in the > parameters, for example. From an SDP perspective, it does what I intend it to. You can change the number of channels negotiated through parameters without requiring a new payload type. https://codereview.webrtc.org/2831333002/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine.cc (left): https://codereview.webrtc.org/2831333002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:1531: }()); On 2017/04/21 12:49:00, kwiberg-webrtc wrote: > Why are you removing this check? I thought we weren't allowed to remove or > change existing payload type mappings? I'm removing this check because the assertion is not valid; I can pass SDP into the JS API in Chrome that violates it. I do add a check for payload types being remapped in this CL. But if I do "setLocalDescription(lotsOfCodecs)" then "setLocalDescription(fewerCodecs)", there's nothing that prevents this from being called with fewer codecs. https://codereview.webrtc.org/2831333002/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2831333002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:1883: } On 2017/04/21 12:49:00, kwiberg-webrtc wrote: > Hmm... here you disallow changing (but not removing) pt -> codec mappings. But > that will still allow a removal immediately followed by reusing the now vacant > pt for a new codec, which has the same effect as changing the map. True. We should probably store a permanent history of mappings. But for this CL, I just wanted to make sure there was *some* equivalent level of error-checking as there was before.
Regardless of whether the individual changes are a good idea or not, this CL does two separate things: allow two payload types to map to the same receive codec, and allow existing payload type mappings to be removed. https://codereview.webrtc.org/2831333002/diff/1/webrtc/api/audio_codecs/audio... File webrtc/api/audio_codecs/audio_format.h (right): https://codereview.webrtc.org/2831333002/diff/1/webrtc/api/audio_codecs/audio... webrtc/api/audio_codecs/audio_format.h:47: bool Matches(const SdpAudioFormat& o) const; On 2017/04/21 13:02:06, Taylor Brandstetter wrote: > On 2017/04/21 12:49:00, kwiberg-webrtc wrote: > > Is that a good idea in general? Opus communicates the number of channels in > the > > parameters, for example. > > From an SDP perspective, it does what I intend it to. You can change the number > of channels negotiated through parameters without requiring a new payload type. Oh---if the RFCs say that that's the requirement, then great! Provided it actually works, of course. :-) But I guess it will work for Opus at least. https://codereview.webrtc.org/2831333002/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine.cc (left): https://codereview.webrtc.org/2831333002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:1531: }()); On 2017/04/21 13:02:06, Taylor Brandstetter wrote: > On 2017/04/21 12:49:00, kwiberg-webrtc wrote: > > Why are you removing this check? I thought we weren't allowed to remove or > > change existing payload type mappings? > > I'm removing this check because the assertion is not valid; I can pass SDP into > the JS API in Chrome that violates it. > > I do add a check for payload types being remapped in this CL. But if I do > "setLocalDescription(lotsOfCodecs)" then "setLocalDescription(fewerCodecs)", > there's nothing that prevents this from being called with fewer codecs. But isn't the error that higher layers (or this one) ought to test for and politely reject such a request, so that the DCHECK can never trigger? https://codereview.webrtc.org/2831333002/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2831333002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:1883: } On 2017/04/21 13:02:06, Taylor Brandstetter wrote: > On 2017/04/21 12:49:00, kwiberg-webrtc wrote: > > Hmm... here you disallow changing (but not removing) pt -> codec mappings. But > > that will still allow a removal immediately followed by reusing the now vacant > > pt for a new codec, which has the same effect as changing the map. > > True. We should probably store a permanent history of mappings. But for this CL, > I just wanted to make sure there was *some* equivalent level of error-checking > as there was before. Storing a permanent history of mappings sounds much more complex than simply never changing existing mappings.
On 2017/04/21 13:18:58, kwiberg-webrtc wrote: > Regardless of whether the individual changes are a good idea or not, this CL > does two separate things: allow two payload types to map to the same receive > codec, and allow existing payload type mappings to be removed. The second thing was already allowed, though, and to my knowledge has always been allowed. There was just a misleading DCHECK, and none of our tests happened to hit the DCHECK until the one I added in this CL. https://codereview.webrtc.org/2831333002/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine.cc (left): https://codereview.webrtc.org/2831333002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:1531: }()); On 2017/04/21 13:18:57, kwiberg-webrtc wrote: > On 2017/04/21 13:02:06, Taylor Brandstetter wrote: > > On 2017/04/21 12:49:00, kwiberg-webrtc wrote: > > > Why are you removing this check? I thought we weren't allowed to remove or > > > change existing payload type mappings? > > > > I'm removing this check because the assertion is not valid; I can pass SDP > into > > the JS API in Chrome that violates it. > > > > I do add a check for payload types being remapped in this CL. But if I do > > "setLocalDescription(lotsOfCodecs)" then "setLocalDescription(fewerCodecs)", > > there's nothing that prevents this from being called with fewer codecs. > > But isn't the error that higher layers (or this one) ought to test for and > politely reject such a request, so that the DCHECK can never trigger? No; apps should be able to set a new local description later with a smaller set of codecs. One could argue that removing codecs from SDP shouldn't remove mappings at the AudioReceiveStream level. But that sort of change would be better in a separate CL.
https://codereview.webrtc.org/2831333002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2831333002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1853: new_codecs.push_back(codec); This warning will trigger if you try to map a fresh payload type to a previously used codec, right? That doesn't seem right. Can you just remove this check altogether? It looks like the new check you do below will catch everything that needs catching. (Specifically, can you remove the entire block of code (lines 1850-1863) that constructs |new_codecs|? The early return on line 1862 could be handled by checking if decoder_map_ == decoder_map just before line 1887.) https://codereview.webrtc.org/2831333002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1876: // to a new codec. Add a TODO about checking for clashes with previously mapped payload types too, not just currently mapped ones? https://codereview.webrtc.org/2831333002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1877: auto existing = decoder_map_.find(codec.id); const?
https://codereview.webrtc.org/2831333002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2831333002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1853: new_codecs.push_back(codec); On 2017/04/24 08:39:34, kwiberg-webrtc wrote: > This warning will trigger if you try to map a fresh payload type to a previously > used codec, right? That doesn't seem right. I wanted to keep the log statement as a warning, because even if it's no longer treated as an error, it's still abnormal and is something I'd want to see in a log when investigating a bug. > Can you just remove this check altogether? It looks like the new check you do > below will catch everything that needs catching. (Specifically, can you remove > the entire block of code (lines 1850-1863) that constructs |new_codecs|? The > early return on line 1862 could be handled by checking if decoder_map_ == > decoder_map just before line 1887.) This seemed to work. No tests are failing, anyway. https://codereview.webrtc.org/2831333002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1876: // to a new codec. On 2017/04/24 08:39:34, kwiberg-webrtc wrote: > Add a TODO about checking for clashes with previously mapped payload types too, > not just currently mapped ones? Done. https://codereview.webrtc.org/2831333002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1877: auto existing = decoder_map_.find(codec.id); On 2017/04/24 08:39:34, kwiberg-webrtc wrote: > const? What's our style for this sort of thing? Just "const auto" doesn't help, since that gives me a "const std::map<int, SdpAudioFormat>::iterator", not an "std::map<int, SdpAudioFormat>::const_iterator".
lgtm Consider re-wording the log statement, but you don't have to. https://codereview.webrtc.org/2831333002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2831333002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1853: new_codecs.push_back(codec); On 2017/04/24 22:44:53, Taylor Brandstetter wrote: > On 2017/04/24 08:39:34, kwiberg-webrtc wrote: > > This warning will trigger if you try to map a fresh payload type to a > previously > > used codec, right? That doesn't seem right. > > I wanted to keep the log statement as a warning, because even if it's no longer > treated as an error, it's still abnormal and is something I'd want to see in a > log when investigating a bug. OK. But the message is no longer accurate, since the codec may still be mapped from its old payload type. Change it to something like "%s mapped a second time (was already at pt %d, now also at pt %d)"? > > Can you just remove this check altogether? It looks like the new check you do > > below will catch everything that needs catching. (Specifically, can you remove > > the entire block of code (lines 1850-1863) that constructs |new_codecs|? The > > early return on line 1862 could be handled by checking if decoder_map_ == > > decoder_map just before line 1887.) > > This seemed to work. No tests are failing, anyway. Well, yes, because all you're doing here is possibly printing a warning and doing an early return if there are no changes in the pt->codec map. I just figured the same job could be accomplished with less code. However, if you really want to keep that log statement, you have to keep all the code that decides when to use it. ... Oh, just saw the newest patch set where you took all my advice. Good! :-) https://codereview.webrtc.org/2831333002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1877: auto existing = decoder_map_.find(codec.id); On 2017/04/24 22:44:54, Taylor Brandstetter wrote: > On 2017/04/24 08:39:34, kwiberg-webrtc wrote: > > const? > > What's our style for this sort of thing? Just "const auto" doesn't help, since > that gives me a "const std::map<int, SdpAudioFormat>::iterator", not an > "std::map<int, SdpAudioFormat>::const_iterator". The style guide basically just says "use const whenever it makes sense". Personally, I find it a good idea to make local variables const whenever I can, since it's very helpful when reading code to see at a glance that a variable will never change. In this particular case, "const auto" *would* be useful---sure, it wouldn't be a const iterator, but the reader will know for sure that you never mutate *the iterator itself*. Making it a const iterator in addition to that would also aid readability, but unfortunately there aren't any low-overhead ways to do that; the best I can come up with is const auto& cmap = decoder_map_; const auto existing = cmap.find(codec.id); and that's too much overhead. But just changing "auto" to "const auto" in the find call *is* worth it, in my opinion. However, it's just a suggestion---if you don't want to, I'm fine with leaving out the "const".
lgtm https://codereview.webrtc.org/2831333002/diff/40001/webrtc/api/audio_codecs/a... File webrtc/api/audio_codecs/audio_format.h (right): https://codereview.webrtc.org/2831333002/diff/40001/webrtc/api/audio_codecs/a... webrtc/api/audio_codecs/audio_format.h:44: // Returns true if this format is compatible with |o|. In SDP terminology, super nit: end in ":", not "," oh well... :) https://codereview.webrtc.org/2831333002/diff/40001/webrtc/api/audio_codecs/a... webrtc/api/audio_codecs/audio_format.h:47: bool Matches(const SdpAudioFormat& o) const; More importantly though, can we deprecate operator==()? And are there current uses of operator==() which should really be Matches()? https://codereview.webrtc.org/2831333002/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2831333002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1853: LOG(LS_WARNING) << codec.name << " payload type changed."; Logging the previous and current PTs would probably be useful in this case - or do we log the new set of mappings in full elsewhere? https://codereview.webrtc.org/2831333002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1870: // Though this check really should happen at a higher level, since this Would that mean that all validation at this level would be removed? Ideally I'd like a situation where we validate input data as early as possible, so that the rest of the system can just DCHECK that it is valid but not have to return errors. Doing validation at multiple levels is both inefficient and error-prone.
https://codereview.webrtc.org/2831333002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2831333002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1853: new_codecs.push_back(codec); On 2017/04/25 09:06:04, kwiberg-webrtc wrote: > On 2017/04/24 22:44:53, Taylor Brandstetter wrote: > > On 2017/04/24 08:39:34, kwiberg-webrtc wrote: > > > This warning will trigger if you try to map a fresh payload type to a > > previously > > > used codec, right? That doesn't seem right. > > > > I wanted to keep the log statement as a warning, because even if it's no > longer > > treated as an error, it's still abnormal and is something I'd want to see in a > > log when investigating a bug. > > OK. But the message is no longer accurate, since the codec may still be mapped > from its old payload type. Change it to something like "%s mapped a second time > (was already at pt %d, now also at pt %d)"? > > > > Can you just remove this check altogether? It looks like the new check you > do > > > below will catch everything that needs catching. (Specifically, can you > remove > > > the entire block of code (lines 1850-1863) that constructs |new_codecs|? The > > > early return on line 1862 could be handled by checking if decoder_map_ == > > > decoder_map just before line 1887.) > > > > This seemed to work. No tests are failing, anyway. > > Well, yes, because all you're doing here is possibly printing a warning and > doing an early return if there are no changes in the pt->codec map. I just > figured the same job could be accomplished with less code. > > However, if you really want to keep that log statement, you have to keep all the > code that decides when to use it. > > ... Oh, just saw the newest patch set where you took all my advice. Good! :-) Fixed log message. https://codereview.webrtc.org/2831333002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1877: auto existing = decoder_map_.find(codec.id); On 2017/04/25 09:06:04, kwiberg-webrtc wrote: > On 2017/04/24 22:44:54, Taylor Brandstetter wrote: > > On 2017/04/24 08:39:34, kwiberg-webrtc wrote: > > > const? > > > > What's our style for this sort of thing? Just "const auto" doesn't help, since > > that gives me a "const std::map<int, SdpAudioFormat>::iterator", not an > > "std::map<int, SdpAudioFormat>::const_iterator". > > The style guide basically just says "use const whenever it makes sense". > Personally, I find it a good idea to make local variables const whenever I can, > since it's very helpful when reading code to see at a glance that a variable > will never change. > > In this particular case, "const auto" *would* be useful---sure, it wouldn't be a > const iterator, but the reader will know for sure that you never mutate *the > iterator itself*. Making it a const iterator in addition to that would also aid > readability, but unfortunately there aren't any low-overhead ways to do that; > the best I can come up with is > > const auto& cmap = decoder_map_; > const auto existing = cmap.find(codec.id); > > and that's too much overhead. But just changing "auto" to "const auto" in the > find call *is* worth it, in my opinion. However, it's just a suggestion---if you > don't want to, I'm fine with leaving out the "const". Done. https://codereview.webrtc.org/2831333002/diff/40001/webrtc/api/audio_codecs/a... File webrtc/api/audio_codecs/audio_format.h (right): https://codereview.webrtc.org/2831333002/diff/40001/webrtc/api/audio_codecs/a... webrtc/api/audio_codecs/audio_format.h:44: // Returns true if this format is compatible with |o|. In SDP terminology, On 2017/04/26 07:44:10, the sun wrote: > super nit: end in ":", not "," > > oh well... :) Done. https://codereview.webrtc.org/2831333002/diff/40001/webrtc/api/audio_codecs/a... webrtc/api/audio_codecs/audio_format.h:47: bool Matches(const SdpAudioFormat& o) const; On 2017/04/26 07:44:09, the sun wrote: > More importantly though, can we deprecate operator==()? And are there current > uses of operator==() which should really be Matches()? It has some existing uses (decoder_database.cc and acm_receiver.cc) that seem intentional. Checking if a format is exactly equal and whether it "matches" both seem like reasonable things to do in different situations. https://codereview.webrtc.org/2831333002/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2831333002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1853: LOG(LS_WARNING) << codec.name << " payload type changed."; On 2017/04/26 07:44:10, the sun wrote: > Logging the previous and current PTs would probably be useful in this case - or > do we log the new set of mappings in full elsewhere? That was suggested by Karl too; done. https://codereview.webrtc.org/2831333002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1870: // Though this check really should happen at a higher level, since this On 2017/04/26 07:44:10, the sun wrote: > Would that mean that all validation at this level would be removed? Ideally I'd > like a situation where we validate input data as early as possible, so that the > rest of the system can just DCHECK that it is valid but not have to return > errors. Doing validation at multiple levels is both inefficient and error-prone. That's the approach I had in mind. We should only need to do input validation at the API level.
The CQ bit was checked by deadbeef@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org, kwiberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2831333002/#ps60001 (title: "Changing log message and comment")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493247885146510,
"parent_rev": "1517caaf7d4d136b35db4fc36050f864913f316d", "commit_rev":
"cb3836773c962c3544bb707c8376e14e64d52f70"}
Message was sent while issue was closed.
Description was changed from ========== Allow a received audio codec's payload type to change. This will create another decoder instance, which isn't ideal, but that's better than the current behavior where things don't work at all. We still need to fix the root cause of the linked bug, which is that we don't remember previous payload type mappings when generating an offer. This CL also adds a check for what is a real error: when a payload type that was mapped to one codec is changed to map to a different codec. And lastly, this CL removes a DCHECK for an assumption that was not valid: that subsequently applied codec lists will always be supersets of previous lists. BUG=webrtc:5847 ========== to ========== Allow a received audio codec's payload type to change. This will create another decoder instance, which isn't ideal, but that's better than the current behavior where things don't work at all. We still need to fix the root cause of the linked bug, which is that we don't remember previous payload type mappings when generating an offer. This CL also adds a check for what is a real error: when a payload type that was mapped to one codec is changed to map to a different codec. And lastly, this CL removes a DCHECK for an assumption that was not valid: that subsequently applied codec lists will always be supersets of previous lists. BUG=webrtc:5847 Review-Url: https://codereview.webrtc.org/2831333002 Cr-Commit-Position: refs/heads/master@{#17897} Committed: https://chromium.googlesource.com/external/webrtc/+/cb3836773c962c3544bb707c8... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/external/webrtc/+/cb3836773c962c3544bb707c8... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
