[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [AVT] I-D ACTION:draft-ietf-avt-rtp-h264-01.txt
Hi.
I have a number of comments on this draft. Most are structure and
clarification request comments.
1. The short copyright statement is missing from the first page. See
ID-Nits http://www.ietf.org/ID-nits.html.
2. The two first pages length is not of the appropriate length.
3. The footer and header is very crowed as the running text uses the
lines directly after and before. To make this more readable and separate
page header and footer from text please leave on empty line between them
and the running text on all pages.
4. I would recommend that you move the changes chapter to last in the
draft and do not include it in the table of contents. That way you can
include it with a note to the RFC editor to remove this section and keep
it being used until the draft is approved.
5. All figures and table are missing number and all references are
either xxxx or xxx for figures respectively tables.
6. Chapter 6.1: The extension bit in the RTP header, shouldn't that also
be used according to RFC 1889 and profile definitions instead of only
the profile?
7. Chapter 6.1: Payload Type. I think that one can use a better language
in this explanation. I especially object to the use of the word
expected. In accordance with today's understanding the text could read:
"The assignment of a payload type needs to be performed either through
the profile used or in a dynamic way.".
8. Chapter 6.2 I think one can clarify the language in the first
paragraph. I would also like to rename it simple payload as this format
are after all only a RTP payload.
Old text:
The RTP payload of a Simple Packet according to this specification
consists of one NALU as depicted in Figure xxxx. The type of the
NALU MUST be specified in [1], i.e., the NALU MUST NOT be an
aggregation packet or a fragmentation unit. A NAL unit stream
composed by decapsulating Simple Packets in RTP sequence number
order MUST conform to the NAL unit decoding order.
New text proposal:
The simple payload defined here MUST contain one and only one NALU of
the types defined in [1]. This means that neither an aggregation payload
or a fragmentation payload can be used. A NAL unit stream composed by
decapsulating Simple Packets in RTP sequence number order MUST conform
to the NAL unit decoding order. The simple payload is shown in figure xxxx.
9. Figure in 6.2: For clarification would it not be better to to show a
RTP header before it:
0 1 2 3
0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
| RTP Header |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
| |
| Single NALU |
| |
| +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
| :...OPTIONAL RTP padding |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
Figure xxxx. RTP payload format for Simple Packet.
10. Chapter 6.3 I have some concerns about the figure structure in this
and the following sub chapters. Combined with the text the complete
picture is not clear. Either you rename the text that says NALU payload
to something clearer or you change the headings of the following sub
chapters to include NALU payload instead of packet in their titles.
11. Chapter 6.3 The use of the F bit for aggregated units. Is it really
wise to set this bit as bad as soon as any of the NALUs part of the
aggregate is bad. This is useless information because you still needs to
go into the packet an look at each NALU to determine which is actually
useful. A cheap implementation that just throws away NALUs which is
damaged can't use this flag anyway. It could be useful it the flag is
instead defined as: "The F bit MUST only be set if all NALUs part of the
aggregate has their F bit set". That way the cheap implementation can
throw the whole packet as soon as it looks at the first bit.
12. Chapter 6.3.1: The DON text paragraph. First it is a very heavy
section of text please look if you could break it into more paragraphs.
A empty line before the numbered list would also be good. Secondly
there is a strange thing in the text that determine if D1 is before D2
or not. " or if D1 > D2 and D1 . D2 >= 32768, then NAL unit ..." the "D1
. D2" should probably be "D1 - D2" or?
13. To clarify how a complete STAP RTP payload looks a example of this
should be provided.
0 1 2 3
0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
| RTP Header |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|STAP NAL HDR | DON | NALU 1 Size |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
| NALU 1 Size | NALU 1 HDR | NALU DATA |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ +
: :
+ +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
| | NALU 2 SIZE | NALU 2 HDR |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
| NALU Data |
| |
| +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
| :...OPTIONAL RTP padding |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
This would be a nice figure in the end of chapter 6.3.1 with a simple
text declaring it to be STAP RTP packet with two NALU.
14. Chapter 6.3.2: Second paragraph. There is missing text about the use
of modular arithmetic. The text says:
"DON of the following NALU is equal to DONB + DOND and MUST NOT be
larger than 65535." Shouldn't this text say that the DON of the NALU is
(DONB+DOND) mod 65535.
15. Chapter 6.3.2: Second paragraph at the end. I think the text
declaring how the timestamp shall be calculated is written to
informative. The text should be more normative and clearly state how it
shall be calculated.
16. At the end of the chapter 6.3.2 I think a complete example should be
shown containing all fields from RTP header to the end. You can never
have to many examples as long as they are correct.
17. Chapter 6.4: I think this chapter is missing some text on the
considerations to make in regards to using FU payloads. What is the
advantage over IP fragmentation? What is the drawbacks? I am also
missing the packetization rules for this format. Even if I later found
them in the next chapter. I think that the rules directly applicable to
FU should be part of this chapter. Otherwise it is hard to understand
that this works. The rules I am talking about is: "Fragments of the same
NALU MUST be sent in consecutive order with no other RTP packets sent
between the first and last fragment" and "Fragments SHALL be reassembled
in RTP sequence number order."
18. Chapter 7.1: Is the rules for how simple packets shall relate to
aggregated packets necessary? Isn't this already covered by bullet
number 2?
19. Chapter 71. The bullet that start with "An Aggregation Packet MUST
succeed a Fragmentation Unit in transmission order if the NAL units in
the Simple Packet precede the NAL unit conveyed in the Fragmentation
Unit in NAL unit decoding order. " Halfway through this sentence there
is a simple packet, shouldn't that be fragment packet.
20. Chapter 8. These depacketization rules seems to not to take possible
packet losses into account. For example:
"If the receiver buffer contains at least N VCL NAL units, NAL units are
removed from the receiver buffer and passed to the decoder in the order
specified below until the buffer contains N-1 VCL NAL units." The quoted
sentence clearly does not take packet loss resulting in lost NAL units.
21. Chapter 9.1."Profile-interoperability". These parameters are they
often set in either direction. If that is the case I would see that
declaring a default value and then spilt out each capability into its
own parameter. Also the name is a bit long.
22. Chapter 9.1: "num-reorder-vcl-NAL-units". First why not rename it to
"interleaving-depth" because this is what this parameter is about.
Secondly the language about the default is strange. The sentence "If the
parameter is not present, num-reorder-VCL-NAL-units equal to 0 MUST be
implied." it seems very unsure even though a MUST is used. I would
formulate it more like "If the parameter is not present the the value of
num-reorder-vcl-NAL-units MUST be set to 0".
23. Chapter 9.1: "aggregation-mode" I would rename this parameter to
what it really represents namely the use of unrestricted mode.
Aggregation is still possible in restricted mode and therefore the
present parameter name is not good. My proposal is "unrestricted". If
value is 1 the unrestricted mode MAY be used. If not present the value
MUST be set to 0. A value of 0 MUST only use restricted mode functionality.
24. Chapter 9.1 Required parameter. The RTP timestamp rate parameter is
missing. I think it is a required parameter unless it is possible to
hard code it to a single value.
25. Chapter 9.2: The second bullet about encoding name. It is missing
text that the RTP timestamp rate shall also be part of the "a=rtpmap" line
26. Chapter 10: One standard thing that usually is part of the security
consideration is if the codec shows any non-linear behavior. If one
knows that switching a single bit in a NALU will result in that the
processing requirements becomes the double or more this should be
expressed.
27. Chapter 14. The correct IETF IPR text is not present. See ID nits:
From RFC 2026 sec 10.4 (A) and 10.4(B) and possibly 10.4(D)
Best Regards
Magnus Westerlund
Multimedia Technologies, Ericsson Research ERA/TVA/A
----------------------------------------------------------------------
Ericsson AB | Phone +46 8 4048287
Torshamsgatan 23 | Fax +46 8 7575550
S-164 80 Stockholm, Sweden | mailto: magnus.westerlund@era.ericsson.se
_______________________________________________
Audio/Video Transport Working Group
avt@ietf.org
https://www1.ietf.org/mailman/listinfo/avt