Draft: draft-ietf-ipdvb-ule-05.txt Review: Michael A. Patton" w Date: 25 maj 2005 ---------------------------------------------------------------- Summary: This draft is basically ready for publication, but has nits that should be fixed before publication. None of the things I noticed in my review (details below) are fatal flaws, but there were so many of them that I think the document needs to be respun at least once before passing it on to the RFC Editor (who can fix the myriad reference problems and typos, but not some of the clarity points), to give them a better base to work from. ---------------------------------------------------------------- There appears to be a technical inconsistency about the meaning of the value in a PP. In the definition of PP in Section 2 says that if the PUSI bit is set, then the PP byte follows the TS header and indicates how many bytes between the end of the header and the start of a PU. Since there's at least the PP byte, it would seem that the minimum value would be 1. But, in Section 3 the next to last paragraph talks about a PP value of 0x00 when the Payload Unit immediately follows the header, and 6.1 seems to repeat that. But it can't because the PP has to be in there. I'm sure there's actually a consistent definition for these fields, but what's in the document isn't quite it. These definitions need to be cleaned up and made more explicit. When I finally got to Section 7.1.1 I find what appears to be the complete definition, which clears it up, but the earlier sections should be cleaned up to be consistent with it. Section 4.4.1 reserves values 0 through 1535 and declares an IANA registry for assigned values. However, in the IANA Consideration section it only talks about values from 0 through 511. I'd suggest adding a paragraph to the IANA Considerations reserving 512 through 1535 for future IETF Standards action. Not that I expect these would ever get used, just that it should probably be explicitly stated. However, Section 5 defines a format for this field that has potential values above 511. So, ultimately I'm completely confused about this field. There is an inconsistency between 4.1 which says D=0 except in an End Indicator. However, 4.5 ascribes meaning for both D=0 and D=1. At first this seems to be a hard inconsistency, however I think (but I'm not the expert here, the authors are supposed to be) what they mean is that in most usage D would be 0, but there may be some cases where D=1 could be needed and that D=0 should be used except when D=1 is absolutely needed. If that's the case, I think a little more explanation in 4.1 could clear up the confusion. I think more explicit description of the distinction in 4.5 would also help. I'm not sure I've completely wrapped my head around the whole thing, but from Section 7.2.1 it looks like this encapsulation assumes that no SNDU will ever need to be spread across more than 2 TS Packets. But, given the length that IP and Ethernet packets can be and that TS Packets are 188 bytes, more than 2 TS packets for each SNDU would probably be the norm. So, I guess I don't understand how the three cases (start of SNDU, middle of SNDU, end of SNDU) are distinguished. So, I think a bit more explanation of that is needed somewhere. Minor comments -------------- The first use of "TS" in the Intro should probably be expanded. It was previously expanded in the Abstract, but you probably shouldn't rely on readers having seen that recently... Throughout the document there are references that get split across line boundaries. This should be avoided. Section 1 has a reference to [draft-ipdvb-arch] which is not in the references section. At the end of Section 1 is a reference to [draft-ipdvb-ar] which is not in the references section. Section 2, in the def for AFC is a reference to [ISO_MPEG] which is not in the references section. Section 2, in the def of MPEG-2 there's mention of H.220 which could usefully have a reference. Section 2, in the def of PID the reference to "all 1s" is easy to misread because the 1 looks like an l in some fonts. I'd suggest writing it as "all ones" to avoid potential confusion. This construction also appears in other places (Section 4.3 at least) which should also be adjusted. Section 2, has two slightly different definitions for PSI. Section 4.6 has a ref to [ITU3563] which is not listed in the references section. Is [ISO-8802-2] really 8802.2 rather than 802.2? RFC3667 and RFC3668 appear in the references section, but are not actually referenced (although 3668 is mentioned in boilerplate that will go away in the RFC). I don't think these belong here and they're certainly not normative. For IEEE 802.3 you use the IEEE ref and mention the ISO one, for 802.2 you only show ISO. I think it might be better to make these references consistent. Typos: Section 1, third line has a double open bracket ("[["). Section 2, in the def of AFC: "ISO_MPEG" => "ISO-MPEG" Section 2, in the def of SI Table, "that is been" => "that has been" Section 2, in the def of TS Header there is a formatting error in the table. In Section 2 the last paragraph (def of ULE Stream) is indented an extra space. Section 2, in the def of ULE Stream: "ISO_MPEG2" => "ISO-MPEG2" Section 4.4 "[IEEE 802.3;" => "[IEEE-802.3;" In Section 5.1 "is of a Mandatory Extension Header" => "is a Mandatory Extension Header" In the diagram at the start of Section 6, the marks on the left and right don't line up quite the same with the lower part. Fun comment... When I got to the def in Section 2 for TS Multiplex and noticed it talking about sending a TS Multiplex over RTP over IP and put that together with the fact that the document was about encoding IP over a TS, I got this great vision of yet another fun circular encapsulation possibility.