1418804 - webRTC: Pass 'maxaveragebitrate' and 'usedtx' from fmtp line to Opus encoder.

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect, P2)

Description

[Thomas Auge]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Ubuntu Chromium/2017.10 Iridium/2017.10 Safari/537.36 Chrome/61.0.3163.100

Steps to reproduce:

- Create session description with createOffer() or createAnswer().

- Modify description's sdp property to include "maxaveragebitrate" in the Opus fmtp line.

- Set description with setLocalDescription() or setRemoteDescription().

- Check actual description with about:webrtc or the RTCPeerConnection.currentLocal/RemoteDescription to confirm that the parameter was ignored.





Actual results:

It appears that most if not all parameters that are not part of the initial SDP are removed. Changing 'stereo' or 'useinbandfec' sticks, but no added parameters do. Bandwidth used is always around 50kbps.


Expected results:

Custom 'maxaveragebitrate' should be reflected in the current SDP and passed on to the Opus encoder. It's a requirement for professional audio or music transmission, which have to minimize the effect of lossy compression.

Comment 1

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

SDP mangling is not expected to be supported. See https://tools.ietf.org/html/draft-ietf-rtcweb-jsep-20#section-5.4

Can you explain more about the actual problem that this remedy is meant to address?

Nils, can you triage this?

Comment 2

[Thomas Auge]

Sections 3.3 and 3.4 of that draft suggest that editing the SDP with JS is intended. To my knowledge it's currently the only way to change any transmission parameter, e.g. bandwidth limits or mono/stereo. But parameters don't stick with setRemoteDescription() either, when they reflect the wishes of the remote end.

Browsing through the FF source code it appears many fmtp parameters including 'maxaveragebitrate' are accounted for to some extent. Is it possible that it's just the parser rejecting additional parameters?

We need this for our product ipDTL.com, but there are a similar services with the same requirements. For recording purposes we have to minimize encoding losses by increasing Opus bandwidth allowance. Our customers use anything between 72 to 320 kbit/s depending on the application.

Comment 3

[Thomas Auge]

Section 5.4 does say it's permitted to edit the SDP to "reduce" capabilities. If the remote end is being more specific about how it would like its Opus served, it fits that description imho, even if increasing bandwidth appears contradictory on the surface.

It would make a lot of sense to have an API for this, but I don't think anything like "RtpTransceiver" exists in practice yet, and since these parameters need to be parsed in the remote description either way, modifying the SDP appears a workable stop gap.

Comment 4

[Nils Ohlmeier [:drno]]

(In reply to Thomas Auge from comment #3)
> Section 5.4 does say it's permitted to edit the SDP to "reduce"
> capabilities. If the remote end is being more specific about how it would
> like its Opus served, it fits that description imho, even if increasing
> bandwidth appears contradictory on the surface.

What :jib is saying is that SDP modification between createOffer() and setLocalDescription() is explicitly forbidden.
What is allowed is that you can modify the SDP to reduce the capabilities before sending it to the answerer. So you would be allowed for example to reduce the value of an existing maxaveragebitrate parameter after calling setLocalDescription, but before sending it to the other side.

In general yes several of the Opus fmtp parameters are not supported by Firefox yet.

If at all we would add support for parsing the "maxaveragebitrate" fmtp parameter on the receiving side.

> It would make a lot of sense to have an API for this, but I don't think
> anything like "RtpTransceiver" exists in practice yet, and since these
> parameters need to be parsed in the remote description either way, modifying
> the SDP appears a workable stop gap.

Transceiver just landed today in bug 1290948.

But this is a codec specific parameter. I don't see how an API call could fit in there. I guess an received maxaveragebitrate from the other side would need to get translated into the max sending bitrate on the sending part of the local Transceiver. But Firefox currently doesn't have support for restricting audio bitrates.

Comment 5

[Thomas Auge]

(In reply to Nils Ohlmeier [:drno] from comment #4)

> What is allowed is that you can modify the SDP to reduce the capabilities
> before sending it to the answerer. So you would be allowed for example to
> reduce the value of an existing maxaveragebitrate parameter after calling
> setLocalDescription, but before sending it to the other side.

For Opus it doesn't matter, but it seems conceptually strange: Shouldn't the local decoder be made aware of a "user" change in its requested format?


> If at all we would add support for parsing the "maxaveragebitrate" fmtp
> parameter on the receiving side.

Assuming you mean the side receiving the SDP, the remote description, that's all we'd need to control the bitrates.


> Transceiver just landed today in bug 1290948.

Oh. Good information, thanks!


> But this is a codec specific parameter. I don't see how an API call could
> fit in there. I guess an received maxaveragebitrate from the other side
> would need to get translated into the max sending bitrate on the sending
> part of the local Transceiver. But Firefox currently doesn't have support
> for restricting audio bitrates.

The fmtp parameter is a custom thing by definition. Since the contents are not meant to be defined on the SDP level, the API could also treat it as just another string. It may not be elegant, but accounting for this on a per-codec level further down the chain may be the only solution ...


For what it's worth: We were able to work around all other differences between Chrome and FF. This is the one thing that stops us from supporting FF.

Comment 6

[bpantazhs]

I have been trying to tackle this bug (btw I tried to get this assigned to me but could not find out how, any help on that would be appreciated) and saw a couple of things: first of all it seems indeed that the fmtp parameters are being parsed correctly but they are not taken into consideration when creating the opusParameters that are ultimately sent into the Opus encoder. This to me looks like a limitation of the JsepAudioCodecDescription class. I was able to modify the code and get the maxaveragebitrate to be set as the mBitrate value, which is a member of the aforementioned class, and this bitrate is passed indeed successfully to the encoder. Now, before I continue with the formal resolution of the bug (I understand there are several steps to this procedure) I would like to ask if my approach is correct.

Thanks!

Comment 7

[Byron Campen [:bwc]]

I do not know if the bitrate param is interpreted as a max average bitrate by webrtc.org, although what is does might be close. There is no setting explicitly for max average bitrate for opus in webrtc.org, as far as I can see. jesup?

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

Passing it directly to Opus should be fine.  Putting it in the codec config should do it; that gets passed directly down into Opus.

Comment 9

[bpantazhs]

Thanks. Could I get this bug assigned to me?

Comment 10

[bpantazhs]

I think I have properly resolved the maxaveragebitrate issue as well as added support for the use_dtx parameter so it would be nice to have this bug assigned to me and pass through the review procedure and get this code into FF as soon as possible.

Comment 11

[Nils Ohlmeier [:drno]]

Sorry that I did not noticed your request earlier.

If you create patch with your changes and attach them here on this bug then we can start the code review. And I also start some try runs for you.

Comment 12

[bpantazhs]

Attachment: Added support for maxaveragebitrate and usedtx parameters

Comment 13

[Nils Ohlmeier [:drno]]

Comment on attachment 8941469 [details] [diff] [review]
Added support for maxaveragebitrate and usedtx parameters

Review of attachment 8941469 [details] [diff] [review]:
-----------------------------------------------------------------

A very good initial patch.

If you could please address my comments and attach a new patch that would be greatly appreciated.

Note: I'm only giving a review - to indicate that this is not ready to land as is. But I think it's pretty close to be able to land it.

::: .lldbinit
@@ +29,5 @@
>  
>  # Dump the current JS stack.
>  command alias js expr DumpJSStack()
> +
> +# Import Mozilla settings and utilities (the awkward structure of these

I think the changes in this file should not get checked in. Can you please remove that?

::: media/libopus/src/opus_encoder.c
@@ -185,4 @@
>      CELTEncoder *celt_enc;
>      int err;
>      int ret, silkEncSizeBytes;
> -

Let's keep this file untouched as well.

::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp
@@ +3578,5 @@
>    ASSERT_EQ((uint32_t)48000, parsed_opus_params.maxplaybackrate);
>    ASSERT_EQ((uint32_t)1, parsed_opus_params.stereo);
>    ASSERT_EQ((uint32_t)0, parsed_opus_params.useInBandFec);
> +  ASSERT_EQ((uint32_t)0, parsed_opus_params.maxaveragebitrate);
> +  ASSERT_EQ((uint32_t)0, parsed_opus_params.useDTX);

On one hand these zeros make more sense here compared to the sdp_unittest values.

But this also illustrates a problem: both of these values appear to be always zero - maybe with the exception when Firefox generates an SDP answer for an offer which included sensible values.
So these need to get set to something else then zero to become useful.

::: media/webrtc/signaling/gtest/sdp_unittests.cpp
@@ +2647,5 @@
>    ASSERT_EQ(32000U, opus_parameters->maxplaybackrate);
>    ASSERT_EQ(1U, opus_parameters->stereo);
>    ASSERT_EQ(1U, opus_parameters->useInBandFec);
> +  ASSERT_EQ(1U, opus_parameters->maxaveragebitrate);
> +  ASSERT_EQ(1U, opus_parameters->useDTX);

This seems wrong to me. The fmtp line doesn't contain DTX or maxaveragebitrate fields. So why would these two parameters be 1? I would expect them to be zero if not present.

::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp
@@ +505,5 @@
>        return kMediaConduitUnknownError;
>      }
>    }
> +    
> +    if (codecConfig->mName == "opus" && codecConfig->mDTXEnabled ) {

The indentation of this appears to be off.

But the bigger change is that this should be rolled into the previous if condition above this. So lets have a single "if (codecConfig->mName == "opus") {" and then in there two separate checks for maxPlaybackRate and DTX.

::: media/webrtc/signaling/src/sdp/SdpAttribute.h
@@ +1413,5 @@
>      {
>        os << "maxplaybackrate=" << maxplaybackrate
>           << ";stereo=" << stereo
> +         << ";useinbandfec=" << useInBandFec
> +         << ";maxaveragebitrate=" << maxaveragebitrate

This will set by default the maxaveragebitrate to zero, since that's the default value. I guess we should only include this parameter if maxaveragebitrate > 0.

@@ +1414,5 @@
>        os << "maxplaybackrate=" << maxplaybackrate
>           << ";stereo=" << stereo
> +         << ";useinbandfec=" << useInBandFec
> +         << ";maxaveragebitrate=" << maxaveragebitrate
> +         << "useDTX=" << useDTX;

Missing leading ';' here.

Comment 14

[bpantazhs]

(In reply to Nils Ohlmeier [:drno] from comment #13)
> Comment on attachment 8941469 [details] [diff] [review]
> Added support for maxaveragebitrate and usedtx parameters
> 
> Review of attachment 8941469 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> A very good initial patch.
> 
> If you could please address my comments and attach a new patch that would be
> greatly appreciated.
> 
> Note: I'm only giving a review - to indicate that this is not ready to land
> as is. But I think it's pretty close to be able to land it.
> 

Thank you for the review Nils. I have a question about the new patch: shall I simply create a new patch with the corrections you propose or shall I roll back the changes and include them in a unified patch and if so, how could I do that?

Also, I have some questions about the corrections:

::: .lldbinit
@@ +29,5 @@
>  
>  # Dump the current JS stack.
>  command alias js expr DumpJSStack()
> +
> +# Import Mozilla settings and utilities (the awkward structure of these

I did not perform any change in this file so I found it strange that it was part of the patch. Could you advise me on how to proceed with this file (as in, manually change it or if there is a way to exclude it from the patch)?

::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp
@@ +3578,5 @@
>    ASSERT_EQ((uint32_t)48000, parsed_opus_params.maxplaybackrate);
>    ASSERT_EQ((uint32_t)1, parsed_opus_params.stereo);
>    ASSERT_EQ((uint32_t)0, parsed_opus_params.useInBandFec);
> +  ASSERT_EQ((uint32_t)0, parsed_opus_params.maxaveragebitrate);
> +  ASSERT_EQ((uint32_t)0, parsed_opus_params.useDTX);

On one hand these zeros make more sense here compared to the sdp_unittest values.

But this also illustrates a problem: both of these values appear to be always zero - maybe with the exception when Firefox generates an SDP answer for an offer which included sensible values.
So these need to get set to something else then zero to become useful.

::: media/webrtc/signaling/gtest/sdp_unittests.cpp
@@ +2647,5 @@
>    ASSERT_EQ(32000U, opus_parameters->maxplaybackrate);
>    ASSERT_EQ(1U, opus_parameters->stereo);
>    ASSERT_EQ(1U, opus_parameters->useInBandFec);
> +  ASSERT_EQ(1U, opus_parameters->maxaveragebitrate);
> +  ASSERT_EQ(1U, opus_parameters->useDTX);

This seems wrong to me. The fmtp line doesn't contain DTX or maxaveragebitrate fields. So why would these two parameters be 1? I would expect them to be zero if not present.

So does it make sense then to simply remove the maxaveragebitrate and useDTX from the tests as we can not be sure of what value would be useful beforehand? Or if I am wrong on that what values would indeed would make sense?

::: media/webrtc/signaling/src/sdp/SdpAttribute.h
@@ +1413,5 @@
>      {
>        os << "maxplaybackrate=" << maxplaybackrate
>           << ";stereo=" << stereo
> +         << ";useinbandfec=" << useInBandFec
> +         << ";maxaveragebitrate=" << maxaveragebitrate

This will set by default the maxaveragebitrate to zero, since that's the default value. I guess we should only include this parameter if maxaveragebitrate > 0.

So shall I break this serialization into two parts and include a a clause like 
(if maxaveragebitrate > 0)
  os << ";maxaveragebitrate=" << maxaveragebitrate;



Thank you again for the review, it was my first :)

Comment 15

[Nils Ohlmeier [:drno]]

(In reply to bpantazhs from comment #14)
> Thank you for the review Nils. I have a question about the new patch: shall
> I simply create a new patch with the corrections you propose or shall I roll
> back the changes and include them in a unified patch and if so, how could I
> do that?

I would prefer one new patch set with the requested modifications, so that we get one clean patchset.
 
> Also, I have some questions about the corrections:
> 
> ::: .lldbinit
> @@ +29,5 @@
> >  
> >  # Dump the current JS stack.
> >  command alias js expr DumpJSStack()
> > +
> > +# Import Mozilla settings and utilities (the awkward structure of these
> 
> I did not perform any change in this file so I found it strange that it was
> part of the patch. Could you advise me on how to proceed with this file (as
> in, manually change it or if there is a way to exclude it from the patch)?

Hmmm that is strange. You could requests only diffs for the files you touched or remove the unwanted changes from the patch file with an editor before uploading here. These are the two things which come to my mind. There are probably more/other options. You options probably also depend of if you use mercurial or git.

> ::: media/webrtc/signaling/gtest/sdp_unittests.cpp
> @@ +2647,5 @@
> >    ASSERT_EQ(32000U, opus_parameters->maxplaybackrate);
> >    ASSERT_EQ(1U, opus_parameters->stereo);
> >    ASSERT_EQ(1U, opus_parameters->useInBandFec);
> > +  ASSERT_EQ(1U, opus_parameters->maxaveragebitrate);
> > +  ASSERT_EQ(1U, opus_parameters->useDTX);
> 
> This seems wrong to me. The fmtp line doesn't contain DTX or
> maxaveragebitrate fields. So why would these two parameters be 1? I would
> expect them to be zero if not present.
> 
> So does it make sense then to simply remove the maxaveragebitrate and useDTX
> from the tests as we can not be sure of what value would be useful
> beforehand? Or if I am wrong on that what values would indeed would make
> sense?

I would prefer to leave them in. These are valid tests after all. This is parsing this line https://searchfox.org/mozilla-central/source/media/webrtc/signaling/gtest/sdp_unittests.cpp#2586

So either edit that line to include dtx and maxbitrate. Or change the asserts to 0U (I hope/assume that these test don't actually pass right now for you).
 
> ::: media/webrtc/signaling/src/sdp/SdpAttribute.h
> @@ +1413,5 @@
> >      {
> >        os << "maxplaybackrate=" << maxplaybackrate
> >           << ";stereo=" << stereo
> > +         << ";useinbandfec=" << useInBandFec
> > +         << ";maxaveragebitrate=" << maxaveragebitrate
> 
> This will set by default the maxaveragebitrate to zero, since that's the
> default value. I guess we should only include this parameter if
> maxaveragebitrate > 0.
> 
> So shall I break this serialization into two parts and include a a clause
> like 
> (if maxaveragebitrate > 0)
>   os << ";maxaveragebitrate=" << maxaveragebitrate;
> 
> 
> 
> Thank you again for the review, it was my first :)

For easier reading I would probably move the useDTX piece right behind the useInBandFec (the order in the SDP doesn't matter), because that can get included all the time no matter if it is 0 or 1.
And then append the maxaveragebitrate only if greater then zero like you wrote down there already.

Thanks for doing this!

Comment 16

[bpantazhs]

Attachment: Support for maxaveragebitrate and usedtx parameters v2

Comment 17

[bpantazhs]

(In reply to Nils Ohlmeier [:drno] from comment #15)

Nils, I just uploaded the second patch as per your instructions. The only modification I did not do was the modification in the

::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp
@@ +3578,5 @@
>    ASSERT_EQ((uint32_t)48000, parsed_opus_params.maxplaybackrate);
>    ASSERT_EQ((uint32_t)1, parsed_opus_params.stereo);
>    ASSERT_EQ((uint32_t)0, parsed_opus_params.useInBandFec);
> +  ASSERT_EQ((uint32_t)0, parsed_opus_params.maxaveragebitrate);
> +  ASSERT_EQ((uint32_t)0, parsed_opus_params.useDTX);

as it was not clear to me by the second part of the corrections, compared to the first corrections, if in the end we need to change this.

Let me know what you think and thanks again for all your help.

Comment 18

[Nils Ohlmeier [:drno]]

Comment on attachment 8942292 [details] [diff] [review]
Support for maxaveragebitrate and usedtx parameters v2

Review of attachment 8942292 [details] [diff] [review]:
-----------------------------------------------------------------

This looks already a lot better.

The biggest issue right now is that when doing a test call with a Firefox build with this patch asserts and crashes. The reason is that your patch here overwrite our current default opus bitrate of 40000 from here https://searchfox.org/mozilla-central/source/media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp#1994 with 0, when ever the maxaveragebitrate is not present in the SDP - which is always right now.

According to the Opus RFC https://tools.ietf.org/html/rfc7587#section-6.1 we should ignore maxaveragebitrate values below 6000 and above 510000. So if you could add that as a check in JsepCodecDescription.h before setting mBitrate I think this should become functional.

Then the remaining issues are that:
- DTX is always off right now.
- we should add unit tests for dtx and maxaveragebitrate.

::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp
@@ +507,2 @@
>      }
>    }

Thanks. This looks better. Can you please fix the indentation of these two closing brackets?

Comment 19

[bpantazhs]

Attachment: Support for maxaveragebitrate and usedtx parameters v3

Comment 20

[bpantazhs]

Nils, thank you for your corrections, I have added them in this third patch for your reviewing (sorry for the indentation errors that is quite non careful on my part). Let me know what I need to do in order to resolve the remaining issues.

Comment 21

[bpantazhs]

Attachment: Support for maxaveragebitrate and usedtx parameters v3_corrected

Comment 22

[bpantazhs]

I have corrected the patch for 510000 <= and 6000 >= controls of the bitrate values check.

Comment 23

[Nils Ohlmeier [:drno]]

After double checking with some experts it is probably saver to not turn on usedtx by default (so keep it as it is right now). So we will only turn it on if the offerer sets it to on. We might change that at a later point in time. This is also how Chrome handles this currently.

One little issue I initially overlooked is that we should not print 'useDTX' into the SDP, but 'usedtx' instead. Could you please fix that?

I think we should probably also not include the 'usedtx' parameter into the SDP when it has a zero value.

The biggest remain issue from my point of view thought is that I would like to see unit tests for this feature. Let me know if you need guidance on how and where to add unit tests.

Comment 24

[bpantazhs]

Thank you for your input Nils. Just to get it straight and not do any hasty commits I have some questions:

(In reply to Nils Ohlmeier [:drno] from comment #23)
> After double checking with some experts it is probably saver to not turn on
> usedtx by default (so keep it as it is right now). So we will only turn it
> on if the offerer sets it to on. We might change that at a later point in
> time. This is also how Chrome handles this currently.

So no changes in the code for this, correct?

> 
> One little issue I initially overlooked is that we should not print 'useDTX'
> into the SDP, but 'usedtx' instead. Could you please fix that?
> 
> I think we should probably also not include the 'usedtx' parameter into the
> SDP when it has a zero value.

So in the file SdpAttribute.h simply change the << ";useDTX=" << useDTX; to something like

if (useDTX == 1)
  os << ";usedtx=" << useDTX; 

> 
> The biggest remain issue from my point of view thought is that I would like
> to see unit tests for this feature. Let me know if you need guidance on how
> and where to add unit tests.

Yes indeed I would need your guidance on that please.

Comment 25

[Nils Ohlmeier [:drno]]

(In reply to bpantazhs from comment #24)
> Thank you for your input Nils. Just to get it straight and not do any hasty
> commits I have some questions:
> 
> (In reply to Nils Ohlmeier [:drno] from comment #23)
> > After double checking with some experts it is probably saver to not turn on
> > usedtx by default (so keep it as it is right now). So we will only turn it
> > on if the offerer sets it to on. We might change that at a later point in
> > time. This is also how Chrome handles this currently.
> 
> So no changes in the code for this, correct?

Correct. That was just me documenting results from conversations with colleagues in case we later go back and question it got implemented this way :-)
 
> > 
> > One little issue I initially overlooked is that we should not print 'useDTX'
> > into the SDP, but 'usedtx' instead. Could you please fix that?
> > 
> > I think we should probably also not include the 'usedtx' parameter into the
> > SDP when it has a zero value.
> 
> So in the file SdpAttribute.h simply change the << ";useDTX=" << useDTX; to
> something like
> 
> if (useDTX == 1)
>   os << ";usedtx=" << useDTX; 

Exactly.
 
> > 
> > The biggest remain issue from my point of view thought is that I would like
> > to see unit tests for this feature. Let me know if you need guidance on how
> > and where to add unit tests.
> 
> Yes indeed I would need your guidance on that please.

In sdp_unittests.cpp you added two new asserts. I think we should have another test where we assert that the parser in fact correctly parses 'usedtx=1' and some random number for maxaveragebitrate. We should have in the end two places where we do these checks: one where we verify that these values are in fact zero (that exists already in your patchset) and another place where we check for non-null values.

And then we need a test to verify that we in fact will answer with usedtx=1 if it was offered. The best place to do that would be in jsep_session_unittest.cpp. If you take a look at the test where you added two new asserts it create the offer here https://searchfox.org/mozilla-central/rev/38bddf549db0f26d4d7575cf695958ff703a3ed7/media/webrtc/signaling/gtest/jsep_session_unittest.cpp#3357 I would create a new test which does exactly the same thing up to and include the CreateOffer() call. Then I would search and replace in that resulting string 'usedtx=0' with 'usedtx=1'. For example something like here https://searchfox.org/mozilla-central/rev/38bddf549db0f26d4d7575cf695958ff703a3ed7/media/webrtc/signaling/gtest/jsep_session_unittest.cpp#3437 you also then need to SetLocalOffer() and SetRemoteOffer() with your modified offer. Then call CreateAnswer() and assert that the answer contains 'usedtx=1'.

Comment 26

[bpantazhs]

Thanks Nils! Here are some questions so I am sure I follow you before I commit anything:

> In sdp_unittests.cpp you added two new asserts. I think we should have
> another test where we assert that the parser in fact correctly parses
> 'usedtx=1' and some random number for maxaveragebitrate. We should have in
> the end two places where we do these checks: one where we verify that these
> values are in fact zero (that exists already in your patchset) and another
> place where we check for non-null values.

So the assert comparisons to 0 remain and there needs to be added something like:

  ASSERT_EQ(48000U, opus_parameters->maxaveragebitrate);
  ASSERT_EQ(1U, opus_parameters->useDTX);

It is not clear to me where to place these as only in TEST_P(NewSdpTest, CheckFormatParameters)
it seems to check for the parameters' values, could you shed some light on this please?

> And then we need a test to verify that we in fact will answer with usedtx=1
> if it was offered. The best place to do that would be in
> jsep_session_unittest.cpp. If you take a look at the test where you added
> two new asserts it create the offer here
> https://searchfox.org/mozilla-central/rev/
> 38bddf549db0f26d4d7575cf695958ff703a3ed7/media/webrtc/signaling/gtest/
> jsep_session_unittest.cpp#3357 I would create a new test which does exactly
> the same thing up to and include the CreateOffer() call. Then I would search
> and replace in that resulting string 'usedtx=0' with 'usedtx=1'. For example
> something like here
> https://searchfox.org/mozilla-central/rev/
> 38bddf549db0f26d4d7575cf695958ff703a3ed7/media/webrtc/signaling/gtest/
> jsep_session_unittest.cpp#3437 you also then need to SetLocalOffer() and
> SetRemoteOffer() with your modified offer. Then call CreateAnswer() and
> assert that the answer contains 'usedtx=1'.


I think I follow this but I am not entirely sure so as to commit a patch. If I understand correctly you would like to see a new test that looks something like this:



TEST_F(JsepSessionTest, ValidateOfferedDTXParameter)
{
  types.push_back(SdpMediaSection::kAudio);
  types.push_back(SdpMediaSection::kVideo);

  RefPtr<JsepTransceiver> audio(new JsepTransceiver(SdpMediaSection::kAudio));
  audio->mSendTrack.UpdateTrackIds(std::vector<std::string>(1, "offerer_stream"), "a1");
  mSessionOff->AddTransceiver(audio);

  RefPtr<JsepTransceiver> video(new JsepTransceiver(SdpMediaSection::kVideo));
  video->mSendTrack.UpdateTrackIds(std::vector<std::string>(1, "offerer_stream"), "v1");
  mSessionOff->AddTransceiver(video);

  std::string offer = CreateOffer();

  size_t start = offer.find("usedtx=0");
  size_t end = offer.find("\r\n", start);
  offer.replace(start, end+2-start, "usedtx=1");

  SetLocalOffer(offer);
  SetRemoteOffer(offer);

  ASSERT_NE(std::string::npos,answer.find("usedtx=1")); 
}


Thanks for all your help!

Comment 27

[bpantazhs]

Nils, it has been a while since our last communication, hope all is well with you. If you have the time please review the above code so that we can move forward with pushing it forward.

Thanks again for all your help so far.

Comment 28

[Thomas Auge]

Thanks for all the effort put into this already. Any chance of this getting picked up again? We're really looking forward to that feature.

Comment 29

[Kevin Leach]

Hi. Any movement on this please? Pro audio streaming web apps aren't going away, and right now all such apps are restricted to Chrome(ium). Do I need to add a bribe (bounty)?

Comment 30

[nkmurali23]

Hi, Same here. Conferencing systems hosting more than 1000 - 4000+ WebRTC clients, we can save lot of bandwidth. With out this bitrate support, we are facing lot pressure from customers. Any idea, when you are planning to support this features???

Comment 31

[Byron Campen [:bwc]]

(In reply to bpantazhs from comment #26)

Thanks Nils! Here are some questions so I am sure I follow you before I
commit anything:

In sdp_unittests.cpp you added two new asserts. I think we should have
another test where we assert that the parser in fact correctly parses
'usedtx=1' and some random number for maxaveragebitrate. We should have in
the end two places where we do these checks: one where we verify that these
values are in fact zero (that exists already in your patchset) and another
place where we check for non-null values.

So the assert comparisons to 0 remain and there needs to be added something
like:

ASSERT_EQ(48000U, opus_parameters->maxaveragebitrate);
ASSERT_EQ(1U, opus_parameters->useDTX);

It is not clear to me where to place these as only in TEST_P(NewSdpTest,
CheckFormatParameters)
it seems to check for the parameters' values, could you shed some light on
this please?

You could modify the SDP that |CheckFormatParameters| uses, by adding maxaveragebitrate and usedtx. Then you can check that the expected values are parsed. Or, you could write a standalone test similar to one of the other test-cases that looks at fmtp (eg; CheckTelephoneEventWithDefaultEvents).

And then we need a test to verify that we in fact will answer with usedtx=1
if it was offered. The best place to do that would be in
jsep_session_unittest.cpp. If you take a look at the test where you added
two new asserts it create the offer here
https://searchfox.org/mozilla-central/rev/
38bddf549db0f26d4d7575cf695958ff703a3ed7/media/webrtc/signaling/gtest/
jsep_session_unittest.cpp#3357 I would create a new test which does exactly
the same thing up to and include the CreateOffer() call. Then I would search
and replace in that resulting string 'usedtx=0' with 'usedtx=1'. For example
something like here
https://searchfox.org/mozilla-central/rev/
38bddf549db0f26d4d7575cf695958ff703a3ed7/media/webrtc/signaling/gtest/
jsep_session_unittest.cpp#3437 you also then need to SetLocalOffer() and
SetRemoteOffer() with your modified offer. Then call CreateAnswer() and
assert that the answer contains 'usedtx=1'.

I think I follow this but I am not entirely sure so as to commit a patch. If
I understand correctly you would like to see a new test that looks something
like this:

TEST_F(JsepSessionTest, ValidateOfferedDTXParameter)
{
types.push_back(SdpMediaSection::kAudio);
types.push_back(SdpMediaSection::kVideo);

RefPtr<JsepTransceiver> audio(new
JsepTransceiver(SdpMediaSection::kAudio));
audio->mSendTrack.UpdateTrackIds(std::vector<std::string>(1,
"offerer_stream"), "a1");
mSessionOff->AddTransceiver(audio);

RefPtr<JsepTransceiver> video(new
JsepTransceiver(SdpMediaSection::kVideo));
video->mSendTrack.UpdateTrackIds(std::vector<std::string>(1,
"offerer_stream"), "v1");
mSessionOff->AddTransceiver(video);

std::string offer = CreateOffer();

size_t start = offer.find("usedtx=0");
size_t end = offer.find("\r\n", start);
offer.replace(start, end+2-start, "usedtx=1");

SetLocalOffer(offer);
SetRemoteOffer(offer);

ASSERT_NE(std::string::npos,answer.find("usedtx=1"));
}

Thanks for all your help!

Sort of, but it could be simpler. See this as an example, since you are only doing audio: https://searchfox.org/mozilla-central/rev/38bddf549db0f26d4d7575cf695958ff703a3ed7/media/webrtc/signaling/gtest/jsep_session_unittest.cpp#4960 Also, you would rewrite the offer only for the remote side, between the SetLocalOffer and SetRemoteOffer calls. Lastly, you will need to create an answer.

Comment 32

[bpantazhs]

(In reply to Byron Campen [:bwc] from comment #31)

You could modify the SDP that |CheckFormatParameters| uses, by adding maxaveragebitrate and usedtx. Then you can check that the expected values are parsed. Or, you could write a standalone test similar to one of the other test-cases that looks at fmtp (eg; CheckTelephoneEventWithDefaultEvents).

I'd like to simply add what's necessary to CheckFormatParameters however since I have already added a check it's not clear to me what more is necessary. Could you provide me with an example please?

Sort of, but it could be simpler. See this as an example, since you are only doing audio: https://searchfox.org/mozilla-central/rev/38bddf549db0f26d4d7575cf695958ff703a3ed7/media/webrtc/signaling/gtest/jsep_session_unittest.cpp#4960 Also, you would rewrite the offer only for the remote side, between the SetLocalOffer and SetRemoteOffer calls. Lastly, you will need to create an answer.

So you are suggesting something like this:

TEST_F(JsepSessionTest, ValidateOfferedDTXParameter)
{
types.push_back(SdpMediaSection::kAudio);
AddTracks(*mSessionOff, "audio");
AddTracks(*mSessionAns, "audio");
std::string offer = CreateOffer();
SetLocalOffer(offer);
size_t start = offer.find("usedtx=0");
size_t end = offer.find("\r\n", start);
offer.replace(start, end+2-start, "usedtx=1");
SetRemoteOffer(offer);
std::string answer = CreateAnswer();
ASSERT_EQ(std::string::npos, answer.find("usedtx=1")); // is this what we need from the answer?
}

Also I have a question about patching. I no longer possess the Firefox code that was used to create the above patches. I do have the changed webrtc files though. What would be the simplest way to recreate this code base in order to be able to submit an additional patch (when that is completed)?

Thank you for your help!

Comment 33

[Byron Campen [:bwc]]

(In reply to bpantazhs from comment #32)

(In reply to Byron Campen [:bwc] from comment #31)

You could modify the SDP that |CheckFormatParameters| uses, by adding maxaveragebitrate and usedtx. Then you can check that the expected values are parsed. Or, you could write a standalone test similar to one of the other test-cases that looks at fmtp (eg; CheckTelephoneEventWithDefaultEvents).

I'd like to simply add what's necessary to CheckFormatParameters however since I have already added a check it's not clear to me what more is necessary. Could you provide me with an example please?

So you already know how to check the values of these new parameters, all that remains is to add them to kH264AudioVideoOffer, and then check that you can get the right values in CheckFormatParameters.

Sort of, but it could be simpler. See this as an example, since you are only doing audio: https://searchfox.org/mozilla-central/rev/38bddf549db0f26d4d7575cf695958ff703a3ed7/media/webrtc/signaling/gtest/jsep_session_unittest.cpp#4960 Also, you would rewrite the offer only for the remote side, between the SetLocalOffer and SetRemoteOffer calls. Lastly, you will need to create an answer.

So you are suggesting something like this:

TEST_F(JsepSessionTest, ValidateOfferedDTXParameter)
{
types.push_back(SdpMediaSection::kAudio);
AddTracks(*mSessionOff, "audio");
AddTracks(*mSessionAns, "audio");
std::string offer = CreateOffer();
SetLocalOffer(offer);
size_t start = offer.find("usedtx=0");
size_t end = offer.find("\r\n", start);
offer.replace(start, end+2-start, "usedtx=1");
SetRemoteOffer(offer);
std::string answer = CreateAnswer();
ASSERT_EQ(std::string::npos, answer.find("usedtx=1")); // is this what we need from the answer?
}

Yes, this is closer to what you want.

Also I have a question about patching. I no longer possess the Firefox code that was used to create the above patches. I do have the changed webrtc files though. What would be the simplest way to recreate this code base in order to be able to submit an additional patch (when that is completed)?

You should clone mozilla-central, apply the patches you have so far on this bug, fix up any conflicts, and work from there.

Comment 34

[bpantazhs]

(In reply to Byron Campen [:bwc] from comment #33)

(In reply to bpantazhs from comment #32)

(In reply to Byron Campen [:bwc] from comment #31)

You could modify the SDP that |CheckFormatParameters| uses, by adding maxaveragebitrate and usedtx. Then you can check that the expected values are parsed. Or, you could write a standalone test similar to one of the other test-cases that looks at fmtp (eg; CheckTelephoneEventWithDefaultEvents).

I'd like to simply add what's necessary to CheckFormatParameters however since I have already added a check it's not clear to me what more is necessary. Could you provide me with an example please?

So you already know how to check the values of these new parameters, all that remains is to add them to kH264AudioVideoOffer, and then check that you can get the right values in CheckFormatParameters.

Thanks, I think I got the picture, so I need to add those to "a=fmtp:109 maxplaybackrate=32000;stereo=1;useinbandfec=1" . But should I use the value 1 for usedtx and a random value, say 48000 for the maxaveragebitrate (as it has been proposed)? If so what happens to the 0 value case?

Sort of, but it could be simpler. See this as an example, since you are only doing audio: https://searchfox.org/mozilla-central/rev/38bddf549db0f26d4d7575cf695958ff703a3ed7/media/webrtc/signaling/gtest/jsep_session_unittest.cpp#4960 Also, you would rewrite the offer only for the remote side, between the SetLocalOffer and SetRemoteOffer calls. Lastly, you will need to create an answer.

So you are suggesting something like this:

TEST_F(JsepSessionTest, ValidateOfferedDTXParameter)
{
types.push_back(SdpMediaSection::kAudio);
AddTracks(*mSessionOff, "audio");
AddTracks(*mSessionAns, "audio");
std::string offer = CreateOffer();
SetLocalOffer(offer);
size_t start = offer.find("usedtx=0");
size_t end = offer.find("\r\n", start);
offer.replace(start, end+2-start, "usedtx=1");
SetRemoteOffer(offer);
std::string answer = CreateAnswer();
ASSERT_EQ(std::string::npos, answer.find("usedtx=1")); // is this what we need from the answer?
}

Yes, this is closer to what you want.

Thanks, do you think that this is ready for a patch as it is?

Comment 35

[Byron Campen [:bwc]]

You'll need to go ahead and update the patches before we can move forward on this.

Comment 36

[Nils Ohlmeier [:drno]]

Attachment: Bug 1418804: adding support for usedtx and maxaveragebitrate. r=ng

Comment 37

[Thomas Auge]

Thanks for picking this up again, Nils!

Comment 38

[Nils Ohlmeier [:drno]]

Attachment: Bug 1418804: update webrtc-sdp 0.3.5 -> 0.3.6. r=ng

Comment 39

[Nils Ohlmeier [:drno]]

Attachment: Bug 1418804: add support for ptime attributes. r=ng

Depends on D71137

Comment 40

[Nils Ohlmeier [:drno]]

Attachment: Bug 1418804: added ptime attribute to rsdparsa C API crate. r=ng

Depends on D74492

Comment 41

[Nils Ohlmeier [:drno]]

Attachment: Bug 1418804: added support for cbr attribute. r=ng

Depends on D74493

Comment 42

[Nils Ohlmeier [:drno]]

Attachment: Bug 1418804: added unit test coverage for Opus fmpt parameters. r=ng

Depends on D74494

Comment 43

[Nils Ohlmeier [:drno]]

Try run https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8cb34222445c406a7c12b51bf70dc553544ebe5

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9aa0f6298bb3
update webrtc-sdp 0.3.5 -> 0.3.6. r=ng

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8a5be6ac90cb
adding support for usedtx and maxaveragebitrate. r=ng
https://hg.mozilla.org/integration/autoland/rev/e9333131b5cf
add support for ptime attributes. r=ng
https://hg.mozilla.org/integration/autoland/rev/611c3702ed04
added ptime attribute to rsdparsa C API crate. r=ng
https://hg.mozilla.org/integration/autoland/rev/9cd106121925
added support for cbr attribute. r=ng
https://hg.mozilla.org/integration/autoland/rev/d02c2bbda7e1
added unit test coverage for Opus fmpt parameters. r=ng

Comment 46

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/9aa0f6298bb3
https://hg.mozilla.org/mozilla-central/rev/8a5be6ac90cb
https://hg.mozilla.org/mozilla-central/rev/e9333131b5cf
https://hg.mozilla.org/mozilla-central/rev/611c3702ed04
https://hg.mozilla.org/mozilla-central/rev/9cd106121925
https://hg.mozilla.org/mozilla-central/rev/d02c2bbda7e1