[FFmpeg-devel] [PATCH] Fix nonsense non-mod16 AMV flipping code.

[Michael Niedermayer]

On Sun, Apr 29, 2012 at 11:51:37PM +0200, Reimar Döffinger wrote:
> On Sun, Apr 29, 2012 at 11:09:12PM +0200, Michael Niedermayer wrote:
> > On Sun, Apr 29, 2012 at 10:09:52PM +0200, Reimar Döffinger wrote:
> > > On Sun, Apr 29, 2012 at 09:45:42PM +0200, Michael Niedermayer wrote:
> > > > On Sat, Apr 28, 2012 at 11:37:08PM +0200, Reimar Döffinger wrote:
> > > > > It is obviously nonsense since it produces wrong results
> > > > > or even crashes (crashes should be encode-only though).
> > > > > Fixes trac issue #1092.
> > > > > 
> > > > > Signed-off-by: Reimar Döffinger <Reimar.Doeffinger at gmx.de>
> > > > > ---
> > > > >  libavcodec/mjpegdec.c      |    3 +--
> > > > >  libavcodec/mjpegenc.c      |    2 +-
> > > > >  libavcodec/mpegvideo_enc.c |    5 -----
> > > > >  3 files changed, 2 insertions(+), 8 deletions(-)
> > > > > 
> > > > > diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
> > > > > index a841384..fe54b45 100644
> > > > > --- a/libavcodec/mjpegdec.c
> > > > > +++ b/libavcodec/mjpegdec.c
> > > > > @@ -972,8 +972,7 @@ static int mjpeg_decode_scan(MJpegDecodeContext *s, int nb_components, int Ah,
> > > > >          s->coefs_finished[c] |= 1;
> > > > >          if (s->flipped) {
> > > > >              // picture should be flipped upside-down for this codec
> > > > > -            int offset = (linesize[c] * (s->v_scount[i] *
> > > > > -                         (8 * s->mb_height - ((s->height / s->v_max) & 7)) - 1));
> > > > > +            int offset = linesize[c] * (s->v_scount[c] * s->height / s->v_max - 1);
> > > > 
> > > > this will draw above the image i think, is this ok for all MB sizes
> > > > and interlacing ?
> > > 
> > > Well, at least not more than before, and the tests I made looked ok.
> > > But I can't tell for sure. I believe the way this is supposed to work
> > > is like this:
> > > Since EMU_EDGE may not be set, we have 16 pixels extra all around the
> > > picture. These we use to both flip and expand the image (since the codec
> > > is intra-only they aren't really needed for anything else).
> > 
> > the picture size is rounded up first then the edge is added
> > thus there should be more space at the bottom then top.
> 
> Sure. I don't see what that has to do with anything though.

the mb size in jpeg is not fixed at 16, we block larger ones
atm but as soon as someone finds a file that has a larger one we will
add support for it and we will forget that this makes the flip code
exploitable.
also there are interlaced mjpegs, ive not double checked the code
but i would expect them to need 32 lines padding yet there are
just 16 on top

the rounding also doesnt look right, a 101 line 420 jpeg has 51
chroma lines but the new code would use 50


[...]

> We do not need the edge pixels, so those 16 edge pixels on top
> should be enough to compensate for the missing alignment.
> With still the caveat that I might miss something. For the encoder
> there might be an issue if someone sets interlaced dct, which
> will lead to a broken encode anyway though I think.
> I wouldn't object to anyone adding code to just make a copy
> of the frame to be extra safe though (and get rid of the
> annoying !EMU_EDGE requirement).

fliping the pic through a bunch of memcpy should be pretty easy
and would avoid the risks of this fliping code in case we dont get
it right and it is nonsense now, was nonsense before the last change
...

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

No snowflake in an avalanche ever feels responsible. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120430/b40dca44/attachment.asc>