[FFmpeg-devel] [PATCH] fix for rev11962 that broke yuv422 mpeg encoding

[Michael Niedermayer]

On Fri, Feb 22, 2008 at 06:57:09PM +0100, Baptiste Coudurier wrote:
> Hi
> 
> Michael Niedermayer wrote:
> > On Wed, Feb 20, 2008 at 12:57:13AM +0100, christophelorenz wrote:
> >> The following command
> >> ffmpeg -i source -pix_fmt yuv422p -vcodec mpeg2video -intra -qscale 1 
> >> output422.m2v
> >> outputs corrupted chroma since rev 11962.
> >> (source is 720x576, -intra and -qscale make the chroma alignment problem 
> >> easier to see)
> >>
> >> Rev 11962 has changed the linesize of the yuv planes using ALIGN macro to 
> >> align Y on U dimension.
> >> However macro is only designed to align to pow2 values and not various 
> >> image width.
> >> Also after alignment to picture.linesize[1] there's no garantee that the 
> >> linesize[0] is still aligned to a STRIDE_ALIGN multiple anymore.
> >>
> >> Refs in libavcodec/utils.c:
> >> L162:
> >> #define ALIGN(x, a) (((x)+(a)-1)&~((a)-1))
> >> L298:
> >> picture.linesize[0] = ALIGN(picture.linesize[0], picture.linesize[1]);
> >>
> >> I changed the code so that the linesize[1,2,3] are adjusted to upper value 
> >> that is a round fraction of linesize[0],
> >> while preserving alignment to STRIDE_ALIGN for all planes.
> >>
> >> Chris.
> >>
> > 
> >> Index: libavcodec/utils.c
> >> ===================================================================
> >> --- libavcodec/utils.c	(revision 12154)
> >> +++ libavcodec/utils.c	(working copy)
> >> @@ -293,10 +293,11 @@
> >>  
> >>          /* next ensures that linesize= 2^x uvlinesize, that is needed because
> >>           * some MC code assumes it */
> >> +        picture.linesize[0] = ALIGN(picture.linesize[0], STRIDE_ALIGN);
> >>          if (picture.linesize[1])
> >> -            picture.linesize[0] = ALIGN(picture.linesize[0], picture.linesize[1]);
> >> -        else
> >> -            picture.linesize[0] = ALIGN(picture.linesize[0], STRIDE_ALIGN);
> >> +            picture.linesize[1] = picture.linesize[0]/(picture.linesize[0]/picture.linesize[1]);
> >> +        if (picture.linesize[2])
> >> +            picture.linesize[2] = picture.linesize[0]/(picture.linesize[0]/picture.linesize[2]);
> >> +        if (picture.linesize[3])
> >> +            picture.linesize[3] = picture.linesize[0]/(picture.linesize[0]/picture.linesize[3]);
> > 
> > This is completely wrong
> > Some MC code assumes 2*linesize[1] == linesize[0] for YV12 or it did assume,
> > i dont remember, but if it still exists and assumes that, it wil definitly not
> > work with any other factor than 2.
> > So first find out if that code still exists and then either remove the align
> > stuff or fix it properly.
> > Also what you wrote above is code duplication due to:
> > for (i=1; i<4; i++)
> >     picture.linesize[i] = ALIGN(picture.linesize[i], STRIDE_ALIGN);
> > 
> > being straight before your code for picture.linesize[0]
> > 
> 
> Shit, this bug is critical.
> 
> Can someone (preferably the author of the concerned commit) fix this
> issue asap ? If this issue stays too long (it's been one week already),
> I'd like to have the commit reverted, and properly implemented.

The last thing the author said was something like "iam leaving town in a minute
and wont have internet access for the next week" or so.
Also you have my approval to revert whatever is needed.

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

Those who are too smart to engage in politics are punished by being
governed by those who are dumber. -- Plato 
-------------- 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/20080222/a596ab63/attachment.pgp>