Re: [rtcweb] Review of Section 5.3 of draft-ietf-rtcweb-sdp-10

[Suhas Nandakumar <[e-mail]>]

Hello Magnus

 Apologies that this took a while. Please see inline.

On Wed, Jul 4, 2018 at 7:57 AM Magnus Westerlund <
[e-mail]> wrote:

> Hi,
>
> I have reviewed Section 5.3 only with a focus on the simulcast and
> multi-stream aspects. For example I have not cared if the ICE details
> are correct in these examples.
>
> Thanks for the review. I have submitted -11 version that should address
the review comments.


>
> A. Section 5.3.1:
>
> BUNDLE grouping framework enables multiplexing of all the 5 streams
>     (1 audio stream + 4 video streams) over a single RTP Session.
>
> It might be good to use RFC 7656 terminology and be specific in that
> this results in 5 source RTP streams.
>
>
[Suhas] Done


> B. Section 5.3.1.
>
> As 5.3 says that this will use FEC or RTX and this one doesn't maybe be
> explicit that it is not added, or rewrite 5.3.
>

[Suhas] Update the description to mean some examples have FEC/RTX and not
all.
The idea was to have this section with examples that cover all these cases
but
not to have all these in all the examples.

>
> C. section 5.3.1:
>
>     | a=group:LS m0 m1                            | [RFC5888]           |
>
> Is it intentional that video 2 is not included in the lip-synch group?
> May require a intention comment for this case.
>

[Suhas] . The intention was to group one AV in a lip sync
group and other can be a non-interactive stream. Updated the text to
indicate the same.


> D. 5.3.1:
>
>     One video source corresponds to VP8 encoding, while the other
>     corresponds to H.264 encoding.
>
> As the m= block represents a media source, if the need is to provide one
> video camera's images (the media  source) as both VP8 and H.264 where
> each are in two different resolutions, then simulcast can handle that
> fine within a single m= block. So from my perspective this is a wrongly
> constructed example from that premise. Can you please clarify if you
> want two media source, i.e. two cameras, or two encoder formats VP8 and
> H.264, or two resolutions, or any combination of them?
>
> I would recommend this example to be two media sources, with encoding
> simulcast. The resolution can be skipped as the later example includes
> resolution simulcasting.
>
>
[Suhas]. Yes, the example wanted to showcase 2 different media sources (one
encoded
 as vp8 and another as h264, can be natively encoded as well) and  each
being sent at
 2 different resolutions.


> E. Section 5.3.1:
>
>     | a=rtcp-fb:* nack                            | [RFC5104]           |
>
>     | a=rtcp-fb:* nack pli                        | [RFC5104]           |
>
> I would note that generalized NACK as well as picture loss indication
> (PLI) is defined in RFC4585.
>

[Suhas] Done, updated all the references.


>
> F. Section 5.3.2:
>
>     This section shows an SDP Offer/Answer for a session with an audio
>     and a single video source.  The video source is encoded as layered
>     coding at 3 different resolutions based on [RFC5583].  The video
>     m=line shows 3 streams with last stream (payload 100) dependent on
>     streams with payload 96 and 97 for decoding.
>
> Also here use of RFC 7656 terminology to talk about (source) RTP streams
> when applicable would be good.
>

[Suhas] Updated.

>
> G. Section 5.3.2:
>
>    | a=rtpmap:96 H264/90000                      | [RFC6184]           |
>     | a=fmtp:96 profile-level-id=4d0028;          | [RFC6184]H.264      |
>     | packetization-mode=1;max-fr=30;max-fs=8040  | Layer 1             |
>     | a=rtpmap:97 H264/90000                      | [RFC6184]           |
>     | a=fmtp:97 profile-level-                    | [RFC6184] H.264     |
>     | id=4d0028;packetization-mode=1; max-        | Layer 2             |
>     | fr=15;max-fs=1200 |                     |
>     | a=rtpmap:100 H264-SVC/90000                 | [RFC6184]           |
>     | a=fmtp:100 profile-level-                   | [RFC6184]           |
>     | id=4d0028;packetization-mode=1; max- |                     |
>     | fr=30;max-fs=8040 |                     |
>     | a=depend:100 lay m1:96,97                   | [RFC5583]Layer 3    |
>
> I have my doubts about this configuration. First of all as it is SVC in
> Single RTP session mode (SST) I don't think it results in multiple RTP
> streams. I think the answerer will interpret this, which of these
> encodings can support. A non scalable H.264, another non-scalable H.264
> or SVC that can contain a number of layers.
>
> Secondly a=depend is only defined for MST mode in RFC 6190.
>
> You also have the wrong reference for the H-264-SVC a=rtpmap and a=fmtp
> line.
>

[Suhas] Agreed Magnus. On re-reading the example it confused me as well :-)
Update the example to represent SST mode SVC configuration.


>
> H. Section 5.3.3:
>
>     | a=extmap:3 urn:ietf:params:rtp-             | [I-D.ietf-avtext-ri |
>     | hdrext:sdes:rtp-stream-id                   | d]                  |
>
> After this line you should have also this line:
>
>     | a=extmap:4 urn:ietf:params:rtp-             | [I-D.ietf-avtext-ri |
>     | hdrext:sdes:repaired-rtp-stream-id                   |
> d]                  |
>
> This to enable the RTX streams to indicate which source RTP stream they
> are repairing. Add to both offer and answer.
>

[Suhas] Good catch. Added it.


>
> I. Section 5.3.3
>
> Why isn't RTX enabled for the audio also?
>

[Suhas] No Specific reason. These are just examples to show configurations
and thus picked MVP
configurations to show the usage of SDP for scenario at hand than
describing all possible
options.


> J. Section 5.3.3. Answer:
>
>    | m=video 0 UDP/TLS/RTP/SAVPF 98 100 101 103  | BUNDLE accepted     |
>
> Payload types 100 and 101 are undefined in this media description.
>

[Suhas] Thanks for catching this. Matched the answer with the offer now.


>
> K. Section 5.3.4 Answer:
>
>     | a=rtpmap:101 VP8/90000                      | [RFC7741]           |
>
> Wrong media type for the payload should be RTX.
>

[Suhas] Nice find. Fixed it.

>
> L. Section 5.3.4 Answer:
>
> Missing rtcp-fb definitions to enable use of NACK which RTX depends on.
>
>
[Suhas] Agreed. Added it.


>
> M. Section 5.3.5:
>
>   | a=fmtp:101 L=5; D=10; ToP=2; repair-        | [I-D.ietf-payload-f |
>     | window=200000                               | lexible-fec-scheme] |
>     | a=fmtp:103 L=5; D=10; ToP=2; repair-        | [I-D.ietf-payload-f |
>     | window=200000                               | lexible-fec-scheme] |
>
> As the parameters are the same, I don't see a point with having two
> payload types. Two different RTP repair streams can use the same payload
> type. As Flex-FEC can do all the binding using the CSRC field, there are
> also no need to have the repaired-rtp-stream-id header extension for
> this one.
>

[Suhas] Good point. Moved to just one PT. Also i think you intend to
include  repaired-rtp-stream-id for this example too ?


> N. Section 5.3.5:
>
> Does it make sense to still have NACK for a session with FEC. If it
> fails isn't PLI or FIR
>

[Suhas] Dont think its needed. Removed nack and kept pli

>
> Cheers
>
> Magnus Westerlund
>
> ----------------------------------------------------------------------
> Network Architecture & Protocols, Ericsson Research
> ----------------------------------------------------------------------
> Ericsson AB                 | Phone  [phone]
> Torshamnsgatan 23           | Mobile [phone]
> SE-164 80 Stockholm, Sweden | mailto: [e-mail]
> ----------------------------------------------------------------------
>
> _______________________________________________
> rtcweb mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/rtcweb
>