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

[Ivan Schreter]

Michael Niedermayer wrote:
> On Tue, Feb 10, 2009 at 09:36:35PM +0100, Ivan Schreter wrote:
>   
> [...]
>   
>> #6a is the parsing of additional information from H.264 picture to get key 
>> frame flag, recovery count, field picture flag and frame number out of the 
>> stream in parser for latter use in lavf. Currently, only key frame flag can 
>> be transported via picture type, so recovery count and field picture flag 
>> are not yet communicated. These will be needed, though, in implementation 
>> of correct timestamps (and/or frame combining).
>>
>> So please review #6a.
>>
>> Regards,
>>
>> Ivan
>>
>>
>>     
>
> [...]
>   
>> +        ptr= ff_h264_decode_nal(h, buf, &dst_length, &consumed, buf_end - buf);
>>     
>
> this effectively reads through the whole bitstream
> obviously this is not ok and unneeded
>   
??? Why that? Before, you criticized copying too much code instead of 
reusing existing functions. So how'd you do it, so that it also *works* 
correctly? The original patch from you I took for the base namely didn't 
work...

>
>   
>> +        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);
>> +        switch(h->nal_unit_type){
>> +        case NAL_SPS:
>> +            ff_h264_decode_seq_parameter_set(h);
>> +            break;
>> +        case NAL_PPS:
>> +            ff_h264_decode_picture_parameter_set(h, h->s.gb.size_in_bits);
>> +            break;
>> +        case NAL_SEI:
>> +            ff_h264_decode_sei(h);
>> +            break;
>> +        case NAL_IDR_SLICE:
>> +        case NAL_SLICE:
>> +            get_ue_golomb(&h->s.gb);  // skip first_mb_in_slice
>> +            slice_type = get_ue_golomb_31(&h->s.gb) % 5;
>> +            *ppict_type= golomb_to_pict_type[slice_type];
>> +            *precovery_frame_cnt = h->sei_recovery_frame_cnt;
>> +            if (h->nal_unit_type == NAL_IDR_SLICE) {
>> +                /* this is an IDR key frame (mostly at begin of the stream) */
>> +                *pkey_frame = 1;
>> +            } else if (h->sei_recovery_frame_cnt >= 0) {
>> +                /* key frame, since recovery_frame_cnt is set */
>> +                *pkey_frame = 1;
>> +            }
>> +            pps_id= get_ue_golomb(&h->s.gb);
>> +            if(pps_id>=MAX_PPS_COUNT){
>> +                av_log(h->s.avctx, AV_LOG_ERROR, "pps_id out of range\n");
>> +                return -1;
>> +            }
>> +            if(!h->pps_buffers[pps_id]) {
>> +                av_log(h->s.avctx, AV_LOG_ERROR, "non-existing PPS referenced\n");
>> +                return -1;
>> +            }
>> +            h->pps= *h->pps_buffers[pps_id];
>> +            if(!h->sps_buffers[h->pps.sps_id]) {
>> +                av_log(h->s.avctx, AV_LOG_ERROR, "non-existing SPS referenced\n");
>> +                return -1;
>> +            }
>> +            h->sps = *h->sps_buffers[h->pps.sps_id];
>> +            *pframe_num = get_bits(&h->s.gb, h->sps.log2_max_frame_num);
>> +            if (h->sps.frame_mbs_only_flag) {
>> +                /* single picture for a frame */
>> +                *pfield_pict = 0;
>> +            } else {
>> +                if (get_bits1(&h->s.gb)) {   /* field_pict_flag */
>> +                    /*
>> +                     * This frame is divided in two separate field pictures.
>> +                     * Field parity bit follows in picture header, but we
>> +                     * don't need it.
>> +                     */
>> +                    *pfield_pict = 1;
>> +                } else {
>> +                    *pfield_pict = 0;
>> +                }
>> +            }
>>     
>
> split the patch please so that
> IDR-keyframe
> SEI recovery
> field pics
>
> are in 3 seperate patches
> the original patch i wrote was not such an intermingled mix
>   
This is no intermingled mix. In the end, it would look exactly like 
this, whether in 3 patches or in 1 patch. It is an integral 
functionality (well, except for field pic, since this is still unused). 
I don't see the reason to split it (maybe except field pict flag / frame 
number, since this is unused yet), since this will obscure the intent of 
the patch - namely communicating key frames (independent of IDR or 
recovery point).

> [...]
>   
>> +/**
>> + * Set context parameters for lavf.
>> + *
>> + * @param s parser context.
>> + * @param pict_type picture type of the first slice in frame.
>> + * @param field_pict set to 1 for field picture, 0 for frame picture.
>> + * @param key_frame set to 1 for key pictures, 0 for non-key pictures.
>> + * @param recovery_frame_cnt frame count from SEI recovery point if present
>> + *              or -1 otherwise.
>> + * @param frame_num frame number.
>> + */
>> +static inline void h264_set_keyframe(AVCodecParserContext *s,
>> +                              int pict_type,
>> +                              int field_pict,
>> +                              int key_frame,
>> +                              int recovery_frame_cnt,
>> +                              int frame_num)
>> +{
>> +    /*
>> +     * NOTE: Currently, there is no flag to tell libavformat about
>> +     * key frames. Instead, it relies on having pict_type set to I
>> +     * for key frames, so we use this to communicate it. This should
>>     
>
> if your patch contains a hack like this its rejected.
> if theres a bug it has to be fixed, not worked around by such hacks
>   
I agree with you that bugs have to be fixed. However, I can not and will 
not, again, CAN NOT AND WILL NOT, fix all FFmpeg bugs and design errors, 
which are affecting my patches. Sometimes, there is time for a 
workaround, before other code is fixed (there are plenty in FFmpeg). 
Fixing this particular problem would entail adding two new fields (key 
frame and recovery count) to AVCodecParserContext, removing current key 
frame hack in compute_pkt_fields() and changing all format parsers to 
set key frame field, which is a lot of work and given current practices 
extremely high effort to box it through. I simply don't have time for 
that. I just wanted to help to support AVCHD camcorders, since this is 
my use case (and of a lot of other people willing to use AVCHD 
camcorders with Linux).

Your own code is full of FIXME comments and you don't have a problem 
with it. Why do you have a problem with it when someone else writes a 
patch which does the correct thing but due to deficiencies in other 
components needs to do a workaround? You are using different quality 
measurements for your own and foreign code, which I don't find OK.

Don't get me wrong, in general, I agree with you to minimize 
workarounds/hacks in the code, preferably to zero. I also looked at SVN 
log messages and patch sizes in files you maintain and I find it very 
good to have them so cleanly separated. But if you want to have 
progress, you'll have to accept *some* workarounds (which you do in your 
own code).

Since I'm feeling extremely unwelcome by FFmpeg developer community and 
lack of constructive criticism which spans more than a few words, I'm 
strongly considering to stop all my FFmpeg-related activities, make 
private patches that work for me and maintain them for me only (and 
eventually other people interested).

Please think about it, before scaring yet another promising dev 
community member away.

Regards,

Ivan