888569 - SDP: Remove default parameters in fmtp attribute for codec

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Shian-Yow Wu [:swu]]

Some parameters are added by default in fmtp attribute for codec, but they should not be always needed.  For example, when we just set "max-fs", the fmtp attribute for codec will be encoded like below:

  a=fmtp:120 max-fs=300;parameter-add=1;usedtx=0;stereo=0;useinbandfec=0;cbr=0

We should not encode parameters unless they are requested to add.

Comment 1

[Shian-Yow Wu [:swu]]

The "parameter-add" seems for H.264, and other parameters (usedtx, stereo, useinbandfec, cbr) are for OPUS.

These parameters in sdp_fmtp_t of "sdp.h" are defined as bool or u16.

    tinybool                  parameter_add;
    u16                       usedtx;
    u16                       stereo;
    u16                       useinbandfec;
    u16                       cbr;

Since 0 is a valid value to encode, we can fix it by changing them to int16 and set negative value when we don't want to encode them.  Does this make sense?

Comment 2

[Adam Roach [:abr]]

Sorry for taking so long to respond -- this fell between the cracks for me.

The way the SDP code tends to encode invalid or exceptional values is by keeping them an unsigned 16-bit integer, and defining a constant 0xFFFF for the exceptional value.

So, for this case, I would recommend leaving the values of type u16, and adding a define similar to:

#define SDP_FMTP_UNUSED 0xFFFF

Comment 3

[Shian-Yow Wu [:swu]]

Attachment: Patch 1: Set correct initial values for unused FMTP parameters

Hi Adam,

Thanks for your comments, attached is the patch.

1. In There are similar initial settings for unused values, in different way.  We might need to unify them in the future.

    fmtp_p->packetization_mode = 0xff;
    fmtp_p->level_asymmetry_allowed = SDP_INVALID_LEVEL_ASYMMETRY_ALLOWED_VALUE;
    fmtp_p->profile = SDP_INVALID_VALUE;
    fmtp_p->level = SDP_INVALID_VALUE;

2. According to RFC 3984(RTP Payload Format for H.264 Video), the optional parameter "parameter-add" can be either 0 or 1.  The current implementation only adds this parameter when it's TRUE.  So this patch changed the initial value from TRUE to FALSE to remove it from FMTP by default.  To best reflect the spec, I think we should change the type of "parameter-add" from tinybool to u16, and give initial value to SDP_FMTP_UNUSED.  How do you think?

Comment 4

[Shian-Yow Wu [:swu]]

Attachment: Patch 2: Change parameter_add from tinybool to u16

Comment 5

[Adam Roach [:abr]]

Comment on attachment 775599 [details] [diff] [review]
Patch 1: Set correct initial values for unused FMTP parameters

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

looks good to me.

Comment 6

[Adam Roach [:abr]]

Comment on attachment 775602 [details] [diff] [review]
Patch 2: Change parameter_add from tinybool to u16

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

This looks generally good, but it makes it impossible for users of the SDP module to tell the difference between "parameter-add=0" (which means that parameters *cannot* be added) and the absence of a "parameter-add" parameter altogether (which means that parameters *can* be added). The easy solution, as I describe below, is to return TRUE for both "parameter-add=1" and for a missing "parameter-add" parameter.

::: media/webrtc/signaling/src/sipcc/core/sdp/sdp_attr_access.c
@@ +8379,5 @@
>          }
>          sdp_p->conf_p->num_invalid_param++;
>          return (FALSE);
>      } else {
> +        return (attr_p->attr.fmtp.parameter_add == 1);

This is problematic from the perspective of how the "parameter-add" parameter actually works (and is related to the reason that the default had been "TRUE" prior to the other patch). RFC 3984 describes it like this:

       parameter-add:   This parameter MAY be used to signal whether
                        the receiver of this parameter is allowed to
                        add parameter sets in its signaling response
                        using the sprop-parameter-sets MIME parameter.
                        The value of this parameter is either 0 or 1.
                        0 is equal to false; i.e., it is not allowed to
                        add parameter sets.  1 is equal to true; i.e.,
                        it is allowed to add parameter sets.  If the
                        parameter is not present, its value MUST be 1.

What this means is that an absence of the parameter needs to be treated the same as "parameter-add=1" (i.e., it needs to return TRUE). In our case, that means that the application will need to treat 1 and SDP_FMTP_UNUSED the same.

All of which is to say I think this should instead look something like:

  /* Both 1 and SDP_FMTP_UNUSED (parameter not present) should be
     treated as TRUE, per RFC 3984, page 45 */

  return (attr_p->attr.fmtp.parameter_add != 0);

Comment 7

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

I assume this makes sure parameter-add isn't added to anything except an H.264 payload type.  And abr's suggested code at the end of his comment looks right.

Comment 8

[Shian-Yow Wu [:swu]]

(In reply to Adam Roach [:abr] from comment #6)
> What this means is that an absence of the parameter needs to be treated the
> same as "parameter-add=1" (i.e., it needs to return TRUE). In our case, that
> means that the application will need to treat 1 and SDP_FMTP_UNUSED the same.
> 
> All of which is to say I think this should instead look something like:
> 
>   /* Both 1 and SDP_FMTP_UNUSED (parameter not present) should be
>      treated as TRUE, per RFC 3984, page 45 */
> 
>   return (attr_p->attr.fmtp.parameter_add != 0);

Good point, thanks for catching this.

Comment 9

[Shian-Yow Wu [:swu]]

Attachment: Patch 2: Change parameter_add from tinybool to u16

Review comment addressed.

Thanks for Adam and Randell!

Comment 10

[Adam Roach [:abr]]

Comment on attachment 778336 [details] [diff] [review]
Patch 2: Change parameter_add from tinybool to u16

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

Looks good. Thanks!

Comment 11

[Shian-Yow Wu [:swu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/867bbad0ac8b
https://hg.mozilla.org/integration/mozilla-inbound/rev/870b10b72e9e

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/867bbad0ac8b
https://hg.mozilla.org/mozilla-central/rev/870b10b72e9e