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

[Michael Niedermayer]

On Sun, Feb 08, 2009 at 03:20:14PM +0100, Ivan Schreter wrote:
> Hi,
> 
> Michael Niedermayer wrote:
> > On Sun, Feb 08, 2009 at 10:07:23AM +0100, Ivan Schreter wrote:
> >   
> >>> [...]  
> >>>       
> >>>> +
> >>>> +    return 0;
> >>>> +}
> >>>> +
> >>>>  int ff_h264_decode_sei(H264Context *h){
> >>>>      MpegEncContext * const s = &h->s;
> >>>>  @@ -6873,6 +6873,10 @@
> >>>>              if(decode_unregistered_user_data(h, size) < 0)
> >>>>                  return -1;
> >>>>              break;
> >>>> +        case SEI_TYPE_RECOVERY_POINT:
> >>>> +            if(decode_recovery_point(h) < 0)
> >>>> +                return -1;
> >>>> +            break;
> >>>>          default:
> >>>>              skip_bits(&s->gb, 8*size);
> >>>>          }
> >>>> @@ -7340,6 +7344,8 @@
> >>>>      int context_count = 0;
> >>>>       h->max_contexts = avctx->thread_count;
> >>>> +    h->sei_recovery_frame_cnt = -1;
> >>>> +
> >>>>  #if 0
> >>>>      int i;
> >>>>      for(i=0; i<50; i++){
> >>>> @@ -7676,6 +7682,14 @@
> >>>>          } else {
> >>>>              cur->repeat_pict = 0;
> >>>>
> >>>> +            /*
> >>>> +             * TODO: Currently, we only communicate the fact we have a 
> >>>> key
> >>>> +             * frame, but there is no possibility to communicate 
> >>>> recovery
> >>>> +             * frame count. Most probably, recovery frame count is 
> >>>> anyway 0.
> >>>> +             * Add this in the future.
> >>>> +             */
> >>>> +            cur->key_frame = (h->sei_recovery_frame_cnt >= 0);
> >>>>     
> >>>>         
> >>> this could set key_frame == 0 for IDR, which is wrong
> >>>   
> >>>       
> >> Correct, my mistake. This should fix it:
> >>
> >> @@ -7422,6 +7445,7 @@
> >>                 return -1;
> >>             }
> >>             idr(h); //FIXME ensure we don't loose some frames if there is 
> >> reordering
> >> +            h->sei_recovery_frame_cnt = 0;
> >>         case NAL_SLICE:
> >>             init_get_bits(&hx->s.gb, ptr, bit_length);
> >>             hx->intra_gb_ptr=
> >>
> >> OK now?
> >>     
> >
> > now its a hack
> > i mean idr != sei_recovery_frame_cnt
> >
> >   
> True, but IDR == key frame, so implicitly it has recovery frame count == 
> 0. So this will correctly detect it. How else would you do it?

keyframe |= ...


> 
> h->sei_recovery_frame_cnt >= 0 iff it's a key frame. It's a keyframe, 
> iff h->sei_recovery_frame_cnt >= 0. The only thing which might 
> theoretically break it is a SEI recovery point associated with IDR frame 
> having recovery_frame_cnt > 0. But this is not logical, since IDR frame 
> is the begin of coding sequence, so it must be decodable without 
> depending on previous frames. Therefore, I think it's OK so and not a hack.

sei_recovery_frame_cnt implicates the existence of a SEI, IDR does not


> 
> >>> also it does not set the key_frame flag if there is no decode but just a
> >>> AVParser
> >>>   
> >>>       
> >> I don't quite understand what you mean. Parser is addressed by patch #6, 
> >> which handles both field combining and key frame flag. It was very hard to 
> >> split it in two patches, therefore I kept it in one.
> >>     
> >
> > i didnt review #6 as it was big & scary IIRC
> >   
> Umm... It is a little bigger, but not _that_ big. And it's pretty 
> straightforward - after finding a complete packet, this packet is parsed 
> via parse_nal_units(), which is nothing else but the code I got from 
> you, corrected and extended by a few things (namely reading field type 
> and frame number). In parser itself, it then looks if it's an unpaired 
> field picture, and if so, it will not return the packet yet, but note 
> down the position in buffer and relevant picture type info, wait for 
> next packet and pair it with it. As error handling, if an unpaired field 
> comes, it will be discarded, since it can't be combined anyway into a 
> full frame.
> 
> I have commented the code IMHO quite extensively, so it should be easy 
> to understand.

the parsing and combining are seperate things requireing seperate
patches.

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

It is dangerous to be right in matters on which the established authorities
are wrong. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090208/ca42cdf8/attachment.pgp>