[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