[FFmpeg-devel] [PATCH] Video decoder and demuxer for AMV files

[Michael Niedermayer]

On Sat, Sep 29, 2007 at 11:52:58AM +0700, Vladimir Voroshilov wrote:
> 2007/9/29, Michael Niedermayer <michaelni at gmx.at>:
> > On Thu, Sep 27, 2007 at 05:32:23PM +0700, Vladimir Voroshilov wrote:
> > > 2007/9/27, Michael Niedermayer <michaelni at gmx.at>:
> > > > Hi
> > > >
> > > > On Thu, Sep 27, 2007 at 10:53:20AM +0200, Aurelien Jacobs wrote:
> > > > > On Wed, 26 Sep 2007 17:18:27 +0700
> > > > > "Vladimir Voroshilov" <voroshil at gmail.com> wrote:
> > > > >
> > > > > > 2007/9/26, Aurelien Jacobs <aurel at gnuage.org>:
> > > > > > > Vladimir Voroshilov wrote:
> > > > > > >
> > > > > > [...]
> > > > > > > > I forget to say that resulting image is flipped upside-down.
> > > > > > > > Please anybody help me with fixing this.
> > > > > > >
> > > > > > > Short answer: negative stride
> > > > > > >
> > > > > > > Longer answer: Look at this code from mjpeg_decode_scan():
> > > > > > >    ptr = s->picture.data[c] +
> > > > > > >             (((s->linesize[c] * (v * mb_y + y) * 8) +
> > > > > > >             (h * mb_x + x) * 8) >> s->avctx->lowres);
> > > > > > >
> > > > > > > To flip the image upside-down, s->picture.data[c] need to be
> > > > > > > replaced by a pointer to (s->picture.data[c] + size of the image
> > > > > > > buffer - size of one line), and s->linesize[c] must be negative.
> > > > > > >
> > > > > > > That's the basic principle. You will probably need to do other
> > > > > > > small related changes. And ensure that the code still behave
> > > > > > > the same way as it does now for other mjpeg codecs.
> > > > > > >
> > > > > >
> > > > > > Thanks you for info!
> > > > > > I modified mjpeg_decode_scan routine.
> > > > > > Now decoder produces correct (not flipped) picture.
> > > > > >
> > > > > > All code enclosed into codec id check, thus there should not be any
> > > > > > problems with other mjpeg codecs.
> > > > > >
> > > > > > Updated patch is attached.
> > > > > >
> > > > > > Index: libavcodec/mjpegdec.c
> > > > > > ===================================================================
> > > > > > --- libavcodec/mjpegdec.c   (revision 10560)
> > > > > > +++ libavcodec/mjpegdec.c   (working copy)
> > > > > > @@ -662,6 +662,15 @@
> > > > > >      int EOBRUN = 0;
> > > > > >
> > > > > >      if(Ah) return 0; /* TODO decode refinement planes too */
> > > > > > +
> > > > > > +    if(s->avctx->codec->id==CODEC_ID_AMVVIDEO) {
> > > > > > +        //Modify pointers for getting upside-down flipped picture
> > > > > > +        for(i=0; i < nb_components; i++) {
> > > > > > +            int c = s->comp_index[i];
> > > > > > +            s->picture.data[c] += (s->linesize[c] * (s->v_scount[i]
> > > > > > * 8 * s->mb_height - 1));
> > > > > > +            s->linesize[c] *= -1;
> > > > > > +        }
> > > > > > +    }
> > > > > >      for(mb_y = 0; mb_y < s->mb_height; mb_y++) {
> > > > > >          for(mb_x = 0; mb_x < s->mb_width; mb_x++) {
> > > > > >              if (s->restart_interval && !s->restart_count)
> > > > > > @@ -717,6 +726,14 @@
> > > > > >              }
> > > > > >          }
> > > > > >      }
> > > > > > +    if(s->avctx->codec->id==CODEC_ID_AMVVIDEO) {
> > > > > > +        //Restore pointers to original values
> > > > > > +        for(i=0; i < nb_components; i++) {
> > > > > > +            int c = s->comp_index[i];
> > > > > > +            s->picture.data[c] += (s->linesize[c] * (s->v_scount[i]
> > > > > > * 8 * s->mb_height - 1));
> > > > > > +            s->linesize[c] *= -1;
> > > > > > +        }
> > > > > > +    }
> > > > > >      return 0;
> > > > > >  }
> > > > >
> > > > > This is ugly IMO.
> > > >
> > > > same oppinon here ...
> > > >
> > > > also the flip flop use of variables that is have them mean 2 different
> > > > things (that is fliped and not fliped) in different parts of the code
> > > > is unacceptable
> > >
> > > I'm afraid my knowledge in this case is enough only to flip picture
> > > while copying to another buffer after call to ff_mjpeg_decode_frame.
> >
> > use SEPERATE variables do NOT hack s->picture.data and s->linesize and then
> > revert that again a few lines later
> >
> > what you do is like:
> >
> > int length=5;
> > bak=length;
> > length= "abc"
> > printf(length);
> > length=bak;
> >
> > i hope you understand what i mean, that is stored linesize and data is not
> > the same as the possibly fliped linesize and data and this should not be
> > in the same variables
> 
> Did i undestand you right (see patch)?

argh, no!

int linesize= s->linesize;
uint8_t *data[3]= {s->picture.data[0], ...

if(amv){
    linesize= -linesize;
    for(i=0; i<3; i++)
        data[i]= ...
}

use linesize+data

and the patch which introduces the new variables should be seperate
from the one adding amv support

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

Republics decline into democracies and democracies degenerate into
despotisms. -- Aristotle
-------------- 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/20070929/80d434f4/attachment.pgp>