[FFmpeg-devel] [PATCH] FFV1 specification: Merge of FrameHeader01() and GlobalHeader()

[Jerome Martinez]

Le 19/05/2015 21:15, Michael Niedermayer a écrit :
> On Mon, May 18, 2015 at 09:04:01PM +0200, Jerome Martinez wrote:
>
>> FrameHeader01() and GlobalHeader() have a lot of common fields
>> and having a common function + default value for fields unused
>> in previous versions is less complex and more coherent than repeating
>> the common part.
> maybe but calling the GlobalHeader FrameHeader is very confusing
> and wrong

 From my point of view, the GlobalHeader() is still a frame header 
because it still contains the same pieces of information about frames 
(frame width, frame height, frame bit depth...), and it is just not 
repeated (moved from the beginning of the frame to the configuration 
record). During the decoding, it is exactly like if the old 
GlobalHeader() is repeated per keyframe (a global header is equivalent 
to a frame header repeated per keyframe), and actually the exact same 
FFV1Context members are used in ffv1dec.c whatever is the version of the 
FFV1 bitstream.
Actually, and still from my point of view, FrameHeader() is very 
confusing and wrong as much as FrameHeader01() in the original 
specification of v0/1 (e.g. if we have a stream with one keyframe and 
all other frames not keyframes, we have only one FrameHeader01() call 
for all frames, as with GlobalHeader(), so FrameHeader01() is confusing 
in that case) so I don't add more confusion or more wrongness.
I also tried not to change everything, and I just "upgraded" the 
original FrameHeader01() function without renaming it (I only removed 
the versioning).
I also added the following sentence in the Configuration Record 
subsection: " LyX Document It (the Configuration Record) contains the 
frame header used for all frames". Isn't it explicit enough?
And I don't find a better wording for something which is per frame 
(actually not per frame, only per keyframes) in v0/1 and global in v2+.
"Header()"? (I am not a big fan of it because it is too much general)
"Parameters()"?
I argue for not renaming too much functions before reaching a larger 
audience (IETF and so on) and getting their points of view.

> [...]
>
>> @@ -7939,6 +7809,10 @@ pred = j ? initial_states[ i ][j - 1][ k ] : 128
>>   
>>   initial_state[ i ][ j ][ k ] = ( pred + initial_state_delta[ i ][ j ][ k
>>    ] ) & 255
>> +\begin_inset Newline newline
>> +\end_inset
>> +
>> +Inferred to be 0 if not present.
> what is inferred to be 0? initial_state? initial_state_delta?
> i think this could be misunderstood as the paragraph is talking
> about multiple variables

I planned to reword the description of initial_state_delta (separation 
between the bitstream syntax which should be in this subsection and 
formulas which should be in another section dedicated to initial_state 
description) in another patch, but in the meanwhile this could be 
misunderstood, true.
I propose to change to "Theses formulas are not applied if 
initial_state_delta[ i ][ j ][ k ] is not present" or "Theses formulas 
are not applied if LyX Document states_coded is 0" or just remove the line.