[FFmpeg-devel] [PATCH] H.264/AVCHD interlaced fixes

[Ivan Schreter]

Michael Niedermayer wrote:
> On Mon, Feb 16, 2009 at 10:47:00AM +0100, Ivan Schreter wrote:
>   
>> Michael Niedermayer wrote:
>>     
>>> of course ANY method that scans the whole is unacceptable and i mean
>>> unacceptable in the sense that no argument will help you, we will
>>> not read the whole frame to extract 5 fields from the headers in the
>>> first 20 bytes
>>>   
>>>       
>> True. So I suppose something like this would suit your requirements:
>>
>> +    for(;;){
>> +        int src_length, dst_length, consumed, bit_length;
>> +        buf = ff_find_start_code(buf, buf_end, &state);
>> +        /*for(; buf + 3 < buf_end; buf++){
>> +            // This should always succeed in the first iteration.
>> +            if(buf[0] == 0 && buf[1] == 0 && buf[2] == 1)
>> +                break;
>> +        }
>> +        buf += 3;*/
>> +        if(buf >= buf_end)
>> +            break;
>> +        --buf;
>> +        src_length = buf_end - buf;
>> +        switch (buf[0] & 0x1f) {
>> +        case NAL_SLICE:
>> +        case NAL_IDR_SLICE:
>> +            // Do not walk the whole buffer just to decode slice header
>> +            if (src_length > 20)
>> +                src_length = 20;
>> +            break;
>> +        }
>> +        ptr= ff_h264_decode_nal(h, buf, &dst_length, &consumed, 
>> src_length);
>> +        if (ptr==NULL || dst_length < 0)
>> +            break;
>> +        while(ptr[dst_length - 1] == 0 && dst_length > 0)
>> +            dst_length--;
>> +        bit_length= !dst_length ? 0 : (8*dst_length - 
>> ff_h264_decode_rbsp_trailing(h, ptr + dst_length - 1));
>> +
>> +        init_get_bits(&h->s.gb, ptr, bit_length);
>> +        nal = h->nal_unit_type;
>> [...]
>> +        buf += consumed;
>> +    }
>>
>>
>> In this case, everything up to slice header must be decoded fully (no 
>> way around it, since we need to get to the slice header, but it's 
>> normally just peanuts),
>>     
>
> i dont see why, ff_find_start_code() should atomatically skip over it
> not really better but also not worse and it avoids the special case for
> slice/idr
>   
Skipping is not the problem. If I understand the code in decode_nal() 
correctly, it also de-escapes 0 0 3 sequences (emulation byte, H.264, 
7.4.1) to 0 0. I seem to have plenty of that in my sample file. Thus, if 
no de-escaping is done, the headers cannot be correctly decoded. E.g., 
when decoding SEI, instead of SEI 1 (0 0 3 1 sequence in buffer, ptr 
pointing to 3), SEI 3 will be decoded, which is obviously wrong.
> also the whole ff_h264_decode_rbsp_trailing seems unneeded unless i
> miss something
>   
I'm also unsure, whether it is needed. Byte-exact length should be 
actually sufficient for the stuff parsed in parser (SEI, SPS, PPS and 
slice header). I tried removing it and could parse my samples without 
problem, as it seems. OTOH, in the future, some other NALs might need 
bit-exact length to decode without warning and then we'll have to search 
why the warning comes. So maybe we should let it in. What do you think?

>> and then for slice itself, we will decode only 
>> first 20 bytes or so. I need to check exact size needed, but 20 bytes 
>> should be actually enough. The loop terminates at slice header.
>>
>> Please let me know, if you are happy with it, so I can update the patch 
>> appropriately.
>>     
>
> it does look like the correct direction
>   
Well, I hope I could convince you now :-)

> and thanks for not giving up even though some of the things i might
> have said where a little offensive ...
>   
Appologies accepted. I understand that you have tough time with a lot of 
patches to review. However, it's impossible to write an acceptable patch 
on the first go, the code is too complex (or better to say, too 
undocumented :-) for someone from outside. So I'd wish to have somewhat 
more pointers in which direction to go next time.

Regards,

Ivan