[FFmpeg-devel] [RFC] WC3 decoder without AVPaletteControl

Michael Niedermayer michaelni
Sat Nov 20 17:14:25 CET 2010


On Sat, Nov 20, 2010 at 11:01:29AM +0100, Reimar D?ffinger wrote:
> On Sat, Nov 20, 2010 at 05:16:49AM +0100, Michael Niedermayer wrote:
> > On Thu, Nov 18, 2010 at 10:18:08PM +0100, Reimar D?ffinger wrote:
> > > > > > > Index: libavformat/utils.c
> > > > > > > ===================================================================
> > > > > > > --- libavformat/utils.c	(revision 18467)
> > > > > > > +++ libavformat/utils.c	(working copy)
> > > > > > > @@ -277,6 +277,20 @@
> > > > > > >      return ret;
> > > > > > >  }
> > > > > > >  
> > > > > > > +int av_append_packet(ByteIOContext *s, AVPacket *pkt, int size)
> > > > > > > +{
> > > > > > > +    int ret;
> > > > > > > +    int old_size;
> > > > > > > +    if (!pkt->size)
> > > > > > > +        return av_get_packet(s, pkt, size);
> > > > > > > +    old_size = pkt->size;
> > > > > > > +    ret = av_grow_packet(pkt, size);
> > > > > > 
> > > > > > isnt av_grow_packet already checking for pkt->size==0 ?
> > > > > 
> > > > > Yes, but it only allocates a new packet then. av_get_packet also sets
> > > > > pkt->pos. Sure, maybe a bit overkill for a single assignment, just
> > > > > thought it might be more future-proof.
> > > > 
> > > > well, fine leave that then
> > > 
> > > One more try...
> > 
> > is it intended to change the table to dynmically generated in this patch?
> > is the code even smaller than the 256 bytes?
> 
> No, this is actually only meant as documentation.
> I'll add that in a separate patch and keep the table by default.
> 
> > > 9a9e24da63ec55a4343383c70e733205f50f2f9c  wc3_pal.diff
> > > Index: libavcodec/avcodec.h
> > > ===================================================================
> > > --- libavcodec/avcodec.h	(revision 25765)
> > > +++ libavcodec/avcodec.h	(working copy)
> > > @@ -3027,6 +3027,14 @@
> > >  void av_shrink_packet(AVPacket *pkt, int size);
> > >  
> > >  /**
> > > + * Increase packet size, correctly zeroing padding
> > > + *
> > > + * @param pkt packet
> > > + * @param grow_by number of bytes by which to increase the size of the packet
> > > + */
> > > +int av_grow_packet(AVPacket *pkt, int grow_by);
> > 
> > maybe this should be size_t
> 
> Well, av_new_packet uses int as well, so that would be inconsistent IMO.
> And I still don't like the idea of code that behaves differently
> on different architectures.
> Oh, and in case someone wants to test it, this file:
> http://samples.mplayerhq.hu/game-formats/wc3-mve/wc3-successful-mission.mve
> after the patch plays correctly in ffplay, before it broke for a short
> time during a palette change.
> Update patch attached, it also contains a change to SHOT_TAG handling
> so that an invalid value there does not cause the palette plane to
> be uninitialized.

>  libavcodec/avcodec.h   |    8 +++
>  libavcodec/avpacket.c  |   20 ++++++++
>  libavcodec/xan.c       |  106 +++++++++++++++++++++++++++++++++++++++++---
>  libavformat/avformat.h |   11 ++++
>  libavformat/utils.c    |   14 +++++
>  libavformat/wc3movie.c |  116 +++++++------------------------------------------
>  6 files changed, 169 insertions(+), 106 deletions(-)
> d593eae626bf104fa2bb0d7ff16eb0b9ae22ecd4  wc3_pal.diff
> Index: libavcodec/avcodec.h
> ===================================================================
> --- libavcodec/avcodec.h	(revision 25774)
> +++ libavcodec/avcodec.h	(working copy)
> @@ -3027,6 +3027,14 @@
>  void av_shrink_packet(AVPacket *pkt, int size);
>  
>  /**
> + * Increase packet size, correctly zeroing padding
> + *
> + * @param pkt packet
> + * @param grow_by number of bytes by which to increase the size of the packet
> + */
> +int av_grow_packet(AVPacket *pkt, int grow_by);
> +
> +/**
>   * @warning This is a hack - the packet memory allocation stuff is broken. The
>   * packet is allocated if it was not really allocated.
>   */

> Index: libavcodec/avpacket.c
> ===================================================================
> --- libavcodec/avpacket.c	(revision 25774)
> +++ libavcodec/avpacket.c	(working copy)
> @@ -20,6 +20,7 @@
>   */
>  
>  #include "avcodec.h"
> +#include "libavutil/avassert.h"
>  
>  
>  void av_destruct_packet_nofree(AVPacket *pkt)

> @@ -48,7 +49,7 @@
>  int av_new_packet(AVPacket *pkt, int size)
>  {
>      uint8_t *data= NULL;
> -    if((unsigned)size < (unsigned)size + FF_INPUT_BUFFER_PADDING_SIZE)
> +    if((unsigned)size <= INT_MAX - FF_INPUT_BUFFER_PADDING_SIZE)
>          data = av_malloc(size + FF_INPUT_BUFFER_PADDING_SIZE);
>      if (data){
>          memset(data + size, 0, FF_INPUT_BUFFER_PADDING_SIZE);

i think av_malloc alraedy protects us against this, that said if you prefer
then this change is surely ok as a seperate commit


> @@ -71,6 +72,23 @@
>      memset(pkt->data + size, 0, FF_INPUT_BUFFER_PADDING_SIZE);
>  }
>  
> +int av_grow_packet(AVPacket *pkt, int grow_by)
> +{
> +    void *new_ptr;
> +    av_assert0((unsigned)pkt->size <= INT_MAX - FF_INPUT_BUFFER_PADDING_SIZE);
> +    if (!pkt->size)
> +        return av_new_packet(pkt, grow_by);
> +    if ((unsigned)grow_by > INT_MAX - (pkt->size + FF_INPUT_BUFFER_PADDING_SIZE))
> +        return -1;
> +    new_ptr = av_realloc(pkt->data, pkt->size + grow_by + FF_INPUT_BUFFER_PADDING_SIZE);
> +    if (!new_ptr)
> +        return AVERROR(ENOMEM);
> +    pkt->data = new_ptr;
> +    pkt->size += grow_by;
> +    memset(pkt->data + pkt->size, 0, FF_INPUT_BUFFER_PADDING_SIZE);
> +    return 0;
> +}
> +
>  int av_dup_packet(AVPacket *pkt)
>  {
>      if (((pkt->destruct == av_destruct_packet_nofree) || (pkt->destruct == NULL)) && pkt->data) {

that should also be seperate from xan/wc* ideally

    

> Index: libavformat/avformat.h
> ===================================================================
> --- libavformat/avformat.h	(revision 25774)
> +++ libavformat/avformat.h	(working copy)
> @@ -245,6 +245,17 @@
>  int av_get_packet(ByteIOContext *s, AVPacket *pkt, int size);
>  
>  
> +/**
> + * Reads data and appends it to the current content of the AVPacket.
> + * If pkt->size is 0 it behaves like av_get_packet.

This should mention that its inefficient and better to allocate a correctly
sized packet

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

Everything should be made as simple as possible, but not simpler.
-- Albert Einstein
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101120/340b8ef3/attachment.pgp>



More information about the ffmpeg-devel mailing list