[FFmpeg-devel] [PATCH] Electronic Arts TGV decoder

Michael Niedermayer michaelni
Sat Jul 19 04:40:10 CEST 2008


On Sat, Jul 19, 2008 at 11:41:05AM +1000, pross at xvid.org wrote:
> On Thu, Jul 17, 2008 at 11:52:35PM +0200, Michael Niedermayer wrote:
> > On Wed, Jul 16, 2008 at 06:51:15PM +1000, pross at xvid.org wrote:
> > > On Mon, Jul 14, 2008 at 01:27:55PM +0200, Michael Niedermayer wrote:
> > > > On Mon, Jul 14, 2008 at 09:21:38PM +1000, pross at xvid.org wrote:
> > > > > On Sun, Jul 13, 2008 at 12:12:32AM +0200, Michael Niedermayer wrote:
> > > > > > On Sat, Jul 12, 2008 at 04:32:20PM +1000, pross at xvid.org wrote:
> > > > > > > On Thu, Jul 10, 2008 at 01:00:32AM +0200, Michael Niedermayer wrote:
> > > > > > > > On Wed, Jul 09, 2008 at 09:18:13PM +1000, pross at xvid.org wrote:
> > > > > > > > > Hi!
> > > > > > > > > 
> > > > > > > > > Second video codec in the EA series.
> > > > > > > > > 
> > > > > > > > > Samples: http://samples.mplayerhq.hu/game-formats/ea-tgv/
> > > > > > > > > Write-up: http://wiki.multimedia.cx/index.php?title=Electronic_Arts_TGV
> > > > > > > > [...]
> > > > > > > 
> > > > > > > Revised patch enclosed.
> > > > > > [...]
> > > > > Certainly worth testing. How do I "cleanly" allocate an AVFrame with
> > > > > linesize tied to width? (grepping lavc for -v DR1 examples was not 
> > > > > productive...)
> > > > my_frame.data[0]= av_malloc(width*height)
> > > > linesize[0]= width
> > > 
> > > Erm I tried the above, together with data[1]=av_malloc for the a palette, but
> > > received a segfault. Obviously I am neglecting something. Clues welcome.
> > 
> > you arent calling get/release_buffer() ? (you should not)
> > Anyway where does it segfault?
> 
> Solved. I had neglected the 12 byte padding enabled within lzo/av_memcpy_backptr,
> and this was causing heap corruption upon av_free() cleanup.
> 
> > > -    if (cnt > c->out_end - dst) {
> > > -        cnt = FFMAX(c->out_end - dst, 0);
> > > -        c->error |= LZO_OUTPUT_FULL;
> > > -    }
> > > +void av_memcpy_backptr(uint8_t *dst, const uint8_t *src, int cnt, int back) {
> > 
> > Arent back and src redundant relative to each other?
> 
> Affirm. Fixed.
> 
> > [...]
> > > +        if (size1>0) {
> > > +            size -= size1;
> > > +            run = FFMIN(size1, dst_end-dst);
> > > +            memcpy(dst, src, run);
> > > +            dst += run;
> > > +            src += run;
> > > +        }
> > > +
> > > +        if (size2>0) {
> > 
> > > +            if (dst-offset<dst_start)
> > 
> > it should be dst-dst_start < offset because if dst is small nummerically
> > then dst-offset can overflow and result in a large pointer.
> 
> Fixed.
> -+            if (dst-offset<dst_start)
> ++            if (dst<dst_start+offset)
> 
> > [...]
> > > +    if (avctx->get_buffer(avctx, &s->frame)<0) {
> > > +        av_log(avctx, AV_LOG_ERROR, "get_buffer() failed\n");
> > > +        return -1;
> > > +    }
> > > +    s->frame.linesize[0] = s->width;
> > 
> > That might work if you remove CODEC_CAP_DR1, but will break various players
> > if its not removed. The buffers provided by get_buffer with CODEC_CAP_DR1
> > could be in video memory or provided by a specific API like directdraw, SDL
> > or other and might not have a overrideable linesize.
> 
> Roger that.
> 
> Updated decoder and avutil diff enclosed.
> (Note to self: sync changelog and dox)
> 
> -- Peter
> (A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B)

> Index: libavutil/lzo.*

looks ok i think, though reimar is maintainer of that
reimar?


[...]

> +/**
> + * Unpack buffer
> + * @return 0 on success, -1 on critical buffer underflow
> + */
> +static int unpack(const uint8_t *src, const uint8_t *src_end, unsigned char *dst, int width, int height) {
> +    unsigned char *dst_end = dst + width*height;
> +    int size,size1,size2,offset,run;
> +    unsigned char *dst_start = dst;
> +
> +    if (src[0] & 0x01)
> +        src += 5;
> +    else
> +        src += 2;
> +
> +    if (src+3>src_end)
> +        return -1;
> +    size = AV_RB24(src);
> +    src += 3;
> +
> +    while(size>0 && src<src_end) {
> +
> +        /* determine size1 and size2 */
> +        size1 = (src[0] & 3);
> +        if ( src[0] & 0x80 ) {  // 1
> +            if (src[0] & 0x40 ) {  // 11
> +                if ( src[0] & 0x20 ) {  // 111
> +                    if ( src[0] < 0xFC )  // !(111111)
> +                        size1 = (((src[0] & 31) + 1) << 2);
> +                    src++;
> +                    size2 = 0;
> +                } else {  // 110
> +                    offset = ((src[0] & 0x10) << 12) + AV_RB16(&src[1]) + 1;
> +                    size2 = ((src[0] & 0xC) << 6) + src[3] + 5;
> +                    src += 4;
> +                }
> +            } else {  // 10
> +                size1 = ( ( src[1] & 0xC0) >> 6 );
> +                offset = (AV_RB16(&src[1]) & 0x3FFF) + 1;
> +                size2 = (src[0] & 0x3F) + 4;
> +                src += 3;
> +            }
> +        } else {  // 0
> +            offset = ((src[0] & 0x60) << 3) + src[1] + 1;
> +            size2 = ((src[0] & 0x1C) >> 2) + 3;
> +            src += 2;
> +        }
> +
> +
> +        /* fetch strip from src */
> +        if (src+size1>src_end)
> +            break;
> +
> +        if (size1>0) {
> +            size -= size1;
> +            run = FFMIN(size1, dst_end-dst);
> +            memcpy(dst, src, run);
> +            dst += run;
> +            src += run;
> +        }
> +

> +        if (size2>0) {
> +            if (dst<dst_start+offset)

the still isnt safe, dst_start+offset can overflow as well, just think of
the array being at the end of the virtual memory space.
dst_start= 0xFFFFFFFF-array_size
This may be unrealistic i wont dispute that but still ..

is there a problem with the solution i suggested?

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

Why not whip the teacher when the pupil misbehaves? -- Diogenes of Sinope
-------------- 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/20080719/2253c962/attachment.pgp>



More information about the ffmpeg-devel mailing list