Re: [codec] AD Evaluation of draft-ietf-codec-oggopus-09

["Ben Campbell" <[e-mail]>]

Thanks for the response. A few more comments on the substantive section 
inline. I will get to the editorial parts in a separate email. (I 
removed sections that do not seem to need further discussion.)

Thanks!

Ben.

On 11 Dec 2015, at 16:16, Timothy B. Terriberry wrote:

[...]

>
>> - 3, last paragraph:
>> -- "implementations need to be prepared":
>> maybe that should be a MUST or SHOULD?
>
> I think the intent was just to point out that someone might violate 
> the preceding SHOULD. It's not clear what actionable thing we'd be 
> saying they MUST do (it would depend on the particular 
> implementation).

I guess you can say MUST gracefully handle...

That makes me wonder why the end-of-stream flag part is a SHOULD and not 
a MUST, though. Are there reasonable circumstances to violate that 
SHOULD?

>
>> -- "last page SHOULD NOT be a continued packet":
>> Why not MUST NOT? Can you describe reasons someone might reasonably
>> violate this?
>
> The intent was to allow but discourage page-level editing of files 
> (i.e., cutting and splicing files by copying complete pages without 
> examining the packets on them). Even just saving a live stream that 
> drops at some point, you might reasonably expect to just be able to 
> copy the data you received to a file. A MUST here would mean you would 
> have to parse the pages, identify the continued packets from the 
> lacing values, remove the extraneous data from the last page if you 
> found some, repack the page buffer, and recompute the page checksum. 
> That's a lot more machinery, and people would likely just ignore the 
> requirement in practice. See also the discussion at 
> <https://www.ietf.org/mail-archive/web/codec/current/msg03145.html>.

As stated, it still requires that extra work, unless the implementor has 
a really good reason and fully understands the consequences. In this 
case, it seems to mean SHOULD do that work unless you don't want to.

I guess this really depends on how strongly you want to discourage the 
behavior, and what breaks if people ignore that discouragement. This 
might be better stated as non-normative guidance. Given that an 
implementation has to be able to deal with a continued packet last page 
anyway, does it really matter?

If you want to keep it as a SHOULD NOT, it would be helpful to mention 
the reasonable violations you envision in the text.

>
>> - 4.2, 2nd paragraph:
>> -- "samples which SHOULD be skipped": Why not MUST?  (also, 
>> s/which/that)
>
> Some specific applications might have a reason for looking at that 
> data (for example, when you have multiple files that you know were 
> produced by chopping up a single existing file without re-encoding).

It would be helpful to mention that in the text.

> In practice we haven't needed anything stronger to convince people to 
> implement this correctly (especially when we point them to 
> <https://people.xiph.org/~greg/opus_testvectors/correctness_trimming_nobeeps.opus>).

I'm not don't think that SHOULD vs MUST logic is contemplated by RFC 
2119 :-) It's not so much how big of a stick you need to get people to 
do the right thing as it is how bad do things break if people do the 
wrong thing. (I do agree with the sentiment to not put in MUSTs that you 
know people won't pay attention to, but I don't think thats the case 
here.)

[...]

>
>> - 4.3: Should the reference to [vorbis-trim] be normative? Can the
>> feature be implemented without understanding the referenced doc?
>
> The answer to the second question is "yes", which I think makes the 
> answer to the first question "no". The reason this is here is to tell 
> people familiar with how Vorbis works what mechanism they should use 
> to achieve the same outcome in Opus. But if you're not familiar with 
> Vorbis, then you can safely ignore the whole paragraph.

Okay.

>
>> - 4.6: Should the reference to [seeking] be normative? Can the 
>> section
>> be implemented without understanding it?
>
> It is possible to implement seeking without knowing anything other 
> than what is described in RFC 3533, but the linked documentation does 
> help considerably.
>
> I would argue that the section itself is not normative, in that you 
> could implement seeking with some other method (for example, linearly 
> scanning the whole file on open to build an index, possibly caching 
> such indexes for files that are opened frequently: this would not be 
> my recommended approach, but people have done it in practice). You 
> could also choose to implement only approximate seeking by just 
> guessing a file position and decoding from there (also done in 
> practice by some players, and a common method for other formats like 
> MP3). Or you could choose not to implement seeking at all.

The current wording says "see [seeking] for general implementation 
guidelines." If those are optional guidelines, it would help to say so, 
or describe it as "some example guidelines". OTOH, if you really prefer 
the approach in [seeking], then it still makes sense for it to be 
normative.

A reference required to understand and implement an optional feature 
should still be normative.

[...]

>
>> -- "Implementations SHOULD reject ID headers"
>> What does it mean, in context, to "reject" headers?
>
> The same thing as rejecting a stream (i.e., don't try to play/decode 
> it). Changed to say that instead.

Which "that"?

Maybe this is a conventional way to say things in the codec community, 
but when I see "reject" I usually think that means you generate some 
sort of error back to the source of the thing you reject. (As opposed to 
"ignore")

>
>> - 5.1.1.4: Why not MUST?
>
> I think the idea was to allow assigning reserved values without 
> explicitly updating this specification. Even though I expect we 
> _would_ want to update this specification, given how long it's taken 
> to get this draft published, I'm wary of adding more pressure to 
> people not to deploy new mappings (because this RFC says they MUST 
> not), even when it looks like they'll be relatively stable and 
> interoperable. At least not for the sole reason that they haven't 
> gotten an RFC number yet.

If this spec reserves those values, then people still need to update it 
to assign them. If you want to make it extensible, then perhaps it needs 
an IANA registry, which, depending on the assignment policy, may not 
require an RFC at all (seems like "expert review" or "specification 
required" might make sense).

Or if you want people to be able to play with other mappings without 
"officially" assigning values, you could make some or all of the range 
"for experimental use".

>
>> - 5.2, last two paragraph: Why are the two SHOULDs not MUSTs?
>
> For the first SHOULD, this is the same as above: some other 
> specification may eventually define the format of the data here and 
> how to modify it, and I didn't think we'd want to make people 
> supporting that specification in violation of this specification. I'm 
> also leaning towards keeping this one a SHOULD since a) the eventual 
> format of this data may apply to all codecs which share the 
> vorbiscomment style of metadata, and b) we would probably try to do 
> this informally first to gain implementation experience, and only 
> bring it to the IETF if there is sufficient interest. I didn't want to 
> make updating this document a prerequisite for that. See also the 
> discussion in this thread: 
> <https://www.ietf.org/mail-archive/web/codec/current/msg03061.html> 
> (skip to 
> <https://www.ietf.org/mail-archive/web/codec/current/msg03107.html> 
> for the resolution of the debate).

I don't think the fact that future specs might change something is an 
appropriate reason to pick SHOULD vs MUST. That's _always_ true, and 
it's what our "UPDATES" process is for. The idea that you might want to 
informally experiment with different approaches is more convincing. But 
if that is the case, it would be helpful to have a sentence to that 
effect in the text.
>
> For the second SHOULD, it felt like the right strength for a judgment 
> call like "excessive". I have no objection to changing it to MUST.

I think the consequences of not following the advice is the important 
point. It sounds like this could be important for DoS attack avoidance, 
and maybe even for buffer-overrun type things. Those fall into the 
category of Bad Things.

(I do agree "Excessive" is a bit vague, so if there were a way to state 
this more concretely it would improve things. But that's true for a 
SHOULD as much as for a MUST).

[...]

>> - 5.2.1, last paragraph: Why is the SHOULD NOT not MUST NOT?
>
> In some contexts, there may be no confusion with multiple 
> normalization schemes. The language in this section was debated a good 
> deal on the list (see threads 
> <https://www.ietf.org/mail-archive/web/codec/current/msg02930.html>, 
> <https://www.ietf.org/mail-archive/web/codec/current/msg03012.html>, 
> and 
> <https://www.ietf.org/mail-archive/web/codec/current/msg03053.html>). 
> I believe everyone is satisfied with the current compromise, and would 
> prefer not to change the strength of any normative statements in it to 
> avoid re-igniting the debate.

I agree that SHOULD NOT may be appropriate here. My point is the text 
should explain why. Almost anytime a SHOULD occurs, it's helpful to 
include text describing why a MUST is not appropriate. (Usually in the 
form of examples or reasons the authors think would be appropriate to 
violate the SHOULD).

>
>> -6, first paragraph:
>> Why are the two SHOULDs not MUSTs? What does it mean to reject a 
>> packet?
>
> For the first SHOULD, the fact that most decoders are likely to reject 
> such packets is enough to discourage encoders from emitting them, and 
> I was wary of imposing a MUST restriction when RFC 6716 chose not to 
> do so.

Did 6716 use a SHOULD?

I assume that any 2119 keywords in this draft are intended to further 
restrict things from 6716. If it's already covered normatively there, 
then it doesn't need 2119 keywords here (unless directly quoted or 
attributed). But in any case, I'm looking for justification in the text. 
(I do consider preventing the use of excessive bandwidth a good enough 
reason for a MUST

>
> For the second, this is the same as the previous comment about 
> excessive allocations. I have no objection to changing this one, 
> either.

I realize now there were 3 SHOULDs in that paragraph. I mean the one 
about avoiding excessive allocations. It sounds like

[...]

>
>> [linear-predection] be normative? Can this be implemented without
>> understanding that? If it does need to be normative, can you find a 
>> more
>> stable reference?
>
> Linear prediction is something that would be understood by "a person 
> having ordinary skill in the art" (to borrow a phrase). We just wanted 
> to include a reference to help those who might not be DSP engineers 
> but wanted to learn more.

Please consider saying something like "For more information on linear 
prediction, see [XXX]." As it's currently stated, it looks like it says 
MAY do X, where the reference specified X, which would require a 
normative reference.

[...]

>
>> - 13:
>> Is this needed? Are the IETF BCP 78 and 79 provisions incorrect for 
>> this?
>
> This text was specifically requested by the Debian project (and I 
> support it personally). The same issue came up for RFC 6716 (for the 
> same reasons). See the discussion at 
> <https://www.ietf.org/mail-archive/web/codec/current/msg02845.html> 
> and 
> <https://www.ietf.org/mail-archive/web/ietf/current/msg73116.html>.
>
> Reviewing those conversations, I do notice that the language of that 
> section was changed after last call for RFC 6716, but the 
> corresponding language in our source repository was not updated 
> (because it was added to the document boilerplate manually by the RFC 
> Editor, and not included in a separate section).

That language is a bit of a problem for a standards track RFC. If people 
are really attached to it, we can pursue allowing it. But I expect 
objections down the line. Keep in mind "NOTE WELL" says that any 
contributions are subject to the rules of BCP 78 and 79.

In any case, 6716 is a bit more code centric , so the discussion made 
more sense there.