[FFmpeg-devel] [PATCH] H.264: fix decoding of plain still images (broken since revision 14289)

[Michael Niedermayer]

On Sun, Jan 04, 2009 at 04:18:10PM +0100, Reinhard Nissl wrote:
> Hi,
> 
> Michael Niedermayer schrieb:
> > On Sat, Jan 03, 2009 at 07:01:32PM +0100, Reinhard Nissl wrote:
> >> Hi,
> >>
> >> the offending change in that revision seems to me the following:
> >>
> >> -            if(prev && pics <= s->avctx->has_b_frames ||
> >> out_of_order)
> >> -                out = prev;
> >> +            if(pics <= s->avctx->has_b_frames || out_of_order)
> >> +                out = NULL;
> >>
> >> In the case of a plain still image, there are no previous images.
> >>  Revision 14288 honored this by checking prev and therefore
> >> output the current image (the still image).
> >> In Revision 14289, this test is missing and therefore, no picture
> >> is output. I've to add that pics as well as has_b_frames was 1
> >> while out_of_order was 0 when checking the condition in both
> >> revisions.
> >>
> >> As you can see from the attached patch, the location of this test
> >> has moved meanwhile and the condition as well as the behavior has
> >> been inverted, but still lacks a test for "no previous image".
> >>
> >> I've added such a test by using outputted_poc and comparing it to
> >> INT_MIN, which is the initial value of this variable.
> >>
> >> For testing, I've uploaded two sample files into subdirectory
> >> rnissl: one with a still frame and one with a pair of fields. See
> >> readme.txt for more information.
> > 
> > this patch totally breaks h264 decoding, thus rejected.
> > id post the list of what it breaks but its as far as i can see
> > every single h264 file from the conformance suite so theres no point.
> > 
> > Besides i do not know what you mean by "still image" which page
> > of the h264 spec describes it?
> 
> Well, didn't have a look into the spec so far, but my sample
> files just contain a single I picture (either a frame or a pair
> of fields). Decoding works without problem but the decoded
> picture is simply not output, starting with that revision.
> 
> In my opinion, this condition has to deal with B pictures.
> has_b_frames has been set due to flag found in the sequence
> header (if I recall correctly), as the streams where these I
> pictures are taken from contain B pictures.
> 
> In the case where this flag would have been not present in the
> stream, pics would be larger than has_b_frames and the picture
> would be output.
> 
> I've now attached a different patch which fixes this issue. It
> checks whether out is an I picture. I hope this change is more
> restrictive than the one from yesterday and doesn't break the
> reference files.

still 9 pages (and i dont mean small 25line pages) of conformance
files failing

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

He who knows, does not speak. He who speaks, does not know. -- Lao Tsu
-------------- 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/20090104/adbb8994/attachment.pgp>