[PATCH] Fix nonsense non-mod16 AMV flipping code.
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@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); data[c] += offset; reference_data[c] += offset; linesize[c] *= -1; diff --git a/libavcodec/mjpegenc.c b/libavcodec/mjpegenc.c index 9326d4d..4788c47 100644 --- a/libavcodec/mjpegenc.c +++ b/libavcodec/mjpegenc.c @@ -466,7 +466,7 @@ static int amv_encode_picture(AVCodecContext *avctx, AVPacket *pkt, //picture should be flipped upside-down for(i=0; i < 3; i++) { - pic.data[i] += (pic.linesize[i] * (s->mjpeg_vsample[i] * (8 * s->mb_height -((s->height/V_MAX)&7)) - 1 )); + pic.data[i] += pic.linesize[i] * (s->mjpeg_vsample[i] * s->height / V_MAX - 1); pic.linesize[i] *= -1; } return ff_MPV_encode_picture(avctx, pkt, &pic, got_packet); diff --git a/libavcodec/mpegvideo_enc.c b/libavcodec/mpegvideo_enc.c index 6d89cfe..bb4be28 100644 --- a/libavcodec/mpegvideo_enc.c +++ b/libavcodec/mpegvideo_enc.c @@ -1012,11 +1012,6 @@ static int load_input_picture(MpegEncContext *s, AVFrame *pic_arg) uint8_t *src = pic_arg->data[i]; uint8_t *dst = pic->data[i]; - if(s->codec_id == CODEC_ID_AMV){ - av_assert0(!(s->avctx->flags & CODEC_FLAG_EMU_EDGE)); - h= ((s->height+15)/16*16)>>v_shift; - } - if (!s->avctx->rc_buffer_size) dst += INPLACE_OFFSET; -- 1.7.10
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.
can someone verify some %16 =! 0 and %8 != 0 files before and after this change on a real AMV device ? CC-ing someone who might be able to test ... [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB There seems to be only one solution to NIH syndrom, ... a shooting squad
On Sun, Apr 29, 2012 at 08:55:31PM +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.
can someone verify some %16 =! 0 and %8 != 0 files before and after this change on a real AMV device ?
Definitely a good idea, however that the old code is nonsense should become obvious if you insert a height of 15, that would leave the data pointers unchanged and invert the stride, i.e. only a single line of the image area would be used. _Subtracting_ the height % 15 just makes no sense whatsoever, though it happens to be almost right for % 8 == 0.
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@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 ? [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Freedom in capitalist society always remains about the same as it was in ancient Greek republics: Freedom for slave owners. -- Vladimir Lenin
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@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). A special check for AMV will ensure we do not use the edge expansion code that would require those extra pixels (necessary since we already used them otherwise). However I certainly won't guarantee that one CODEC_ID_AMV test catches all cases that would ordinarily use extra edge pixels. Plus, I believe it would make more sense for that check to test for intra_only instead of AMV specifically, it's a waste of processing power to fill in edge pixels that are guaranteed to never be used...
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@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. [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Why not whip the teacher when the pupil misbehaves? -- Diogenes of Sinope
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@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. 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).
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@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
On 30 Apr 2012, at 01:53, Michael Niedermayer <michaelni@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@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.
On Mon, Apr 30, 2012 at 07:39:42AM +0200, Reimar Döffinger wrote:
On 30 Apr 2012, at 01:53, Michael Niedermayer <michaelni@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@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
Reimar Döffinger <Reimar.Doeffinger <at> gmx.de> writes:
It is obviously nonsense since it produces wrong results or even crashes (crashes should be encode-only though). Fixes trac issue #1092.
Ping, the ticket is still reproducible. Carl Eugen
Carl Eugen Hoyos <cehoyos <at> ag.or.at> writes:
Reimar Döffinger <Reimar.Doeffinger <at> gmx.de> writes:
It is obviously nonsense since it produces wrong results or even crashes (crashes should be encode-only though). Fixes trac issue #1092.
Ping, the ticket is still reproducible.
It is not reproducible anymore, but the ping still applies. Imo, we are much more likely to get feedback if we allow to encode such samples (with a warning). Carl Eugen
On Sun, Jun 01, 2014 at 07:50:55AM +0000, Carl Eugen Hoyos wrote:
Carl Eugen Hoyos <cehoyos <at> ag.or.at> writes:
Reimar Döffinger <Reimar.Doeffinger <at> gmx.de> writes:
It is obviously nonsense since it produces wrong results or even crashes (crashes should be encode-only though). Fixes trac issue #1092.
Ping, the ticket is still reproducible.
It is not reproducible anymore, but the ping still applies.
Imo, we are much more likely to get feedback if we allow to encode such samples (with a warning).
hmm indeed, changed that way [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB You can kill me, but you cannot change the truth.
On Sun, Jun 01, 2014 at 07:50:55AM +0000, Carl Eugen Hoyos wrote:
Carl Eugen Hoyos <cehoyos <at> ag.or.at> writes:
Reimar Döffinger <Reimar.Doeffinger <at> gmx.de> writes:
It is obviously nonsense since it produces wrong results or even crashes (crashes should be encode-only though). Fixes trac issue #1092.
Ping, the ticket is still reproducible.
It is not reproducible anymore, but the ping still applies.
I have to admit I didn't push this patch further because when I recently stumbled over it I could hardly understand it and had no idea if it is correct :-( I certainly won't object to it being applied, but if someone feels like double-checking the code surrounding AMV and non-mod-16 sizes and if it even makes sense (disregarding now whether it matches behaviour of some reference decoder) that is welcome.
participants (3)
-
Carl Eugen Hoyos -
Michael Niedermayer -
Reimar Döffinger