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

[Reimar Döffinger]

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.

> 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.