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

[Michael Niedermayer]

On Mon, Apr 30, 2012 at 07:39:42AM +0200, Reimar Döffinger wrote:
> On 30 Apr 2012, at 01:53, Michael Niedermayer <michaelni at gmx.at> wrote:
> > 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
> 
> Huh? I only know of progressive JPEG which I think does not have this issue and anyway I believe we do not support encoding either.

our decoder supports interlaced and my concern is on the decoder side,
which is where the overwriting over the top would possible happen


> 
> > the rounding also doesnt look right, a 101 line 420 jpeg has 51
> > chroma lines but the new code would use 50
> 
> I assumed we still disallowed odd sized 420 completely.
> There is no way to flip odd sized 420 that would give correct chroma results.

just tried, encoding and decoding of a 257x257 jpeg works fine
with ffmpeg

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

Many that live deserve death. And some that die deserve life. Can you give
it to them? Then do not be too eager to deal out death in judgement. For
even the very wise cannot see all ends. -- Gandalf
-------------- 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/5be19de2/attachment.asc>