[FFmpeg-devel] [PATCH 2/2] lavf/avienc: Add xxpc entries to index

Michael Niedermayer michael at niedermayer.cc
Sat Mar 12 12:53:28 CET 2016


On Sat, Mar 12, 2016 at 07:14:16AM +0100, Mats Peterson wrote:
> Here's an interesting one. Windows Media Player won't make any palette
> changes without the xxpc chunks beeing indexed.
> 
> Fixing the logic for reading and seeking with xxpc chunks in the
> demuxer  is a future task. Now the muxing of video with xxpc chunks
> works properly at least.
> 
> Try playing the resulting test.avi file from the command line below
> with Windows Media Player, with and without this patch.
> 
> ffmpeg -i TOON.AVI -c:v copy -c:a copy test.avi
> 
> Mats
> 
> -- 
> Mats Peterson
> http://matsp888.no-ip.org/~mats/

>  libavformat/avi.h            |    6 +++-
>  libavformat/avienc.c         |   56 +++++++++++++++++++++++++++++++++----------
>  tests/ref/lavf-fate/avi_cram |    4 +--
>  3 files changed, 51 insertions(+), 15 deletions(-)
> 2cf2565f9e258ee1a2bfcb83e4f30ecb1c13296d  0002-Add-xxpc-entries-to-index.patch
> From 50f6c1dd38f503e77d53e0e6cdbadfe511282126 Mon Sep 17 00:00:00 2001
> From: Mats Peterson <matsp888 at yahoo.com>
> Date: Sat, 12 Mar 2016 07:00:33 +0100
> Subject: [PATCH 2/2] lavf/avienc: Add xxpc entries to index
> 
> ---
>  libavformat/avi.h            |    6 ++++-
>  libavformat/avienc.c         |   56 +++++++++++++++++++++++++++++++++---------
>  tests/ref/lavf-fate/avi_cram |    4 +--
>  3 files changed, 51 insertions(+), 15 deletions(-)
> 
> diff --git a/libavformat/avi.h b/libavformat/avi.h
> index 34da76f..af21f2c 100644
> --- a/libavformat/avi.h
> +++ b/libavformat/avi.h
> @@ -32,7 +32,11 @@
>  #define AVI_MASTER_INDEX_SIZE   256
>  #define AVI_MAX_STREAM_COUNT    100
>  
> +/* stream header flags */
> +#define AVISF_VIDEO_PALCHANGES  0x00010000
> +
>  /* index flags */
> -#define AVIIF_INDEX             0x10
> +#define AVIIF_INDEX             0x00000010
> +#define AVIIF_NO_TIME           0x00000100
>  
>  #endif /* AVFORMAT_AVI_H */
> diff --git a/libavformat/avienc.c b/libavformat/avienc.c
> index ad50379..b731bc2 100644
> --- a/libavformat/avienc.c
> +++ b/libavformat/avienc.c
> @@ -44,13 +44,14 @@
>   */
>  
>  typedef struct AVIIentry {
> -    unsigned int flags, pos, len;
> +    char tag[5];

the tag should be 4 bytes
5 is ugly, it requires padding and bloats the structure with a zero
byte


> +    unsigned int flags;
> +    unsigned int pos;
> +    unsigned int len;
>  } AVIIentry;
>  
>  #define AVI_INDEX_CLUSTER_SIZE 16384
>  
> -#define AVISF_VIDEO_PALCHANGES 0x00010000
> -
>  typedef struct AVIIndex {
>      int64_t     indx_start;
>      int64_t     audio_strm_offset;

> @@ -612,9 +613,13 @@ static int avi_write_idx1(AVFormatContext *s)
>              }
>              if (!empty) {
>                  avist = s->streams[stream_id]->priv_data;
> -                avi_stream2fourcc(tag, stream_id,
> +                if (*ie->tag)

==18406== Conditional jump or move depends on uninitialised value(s)
==18406==    at 0x598D80: avi_write_idx1 (avienc.c:616)
==18406==    by 0x599D6D: avi_write_trailer (avienc.c:859)
==18406==    by 0x64A234: av_write_trailer (mux.c:1124)
==18406==    by 0x43A729: transcode (ffmpeg.c:4173)
==18406==    by 0x43ACE3: main (ffmpeg.c:4334)



> +                    ffio_wfourcc(pb, ie->tag);
> +                else {
> +                    avi_stream2fourcc(tag, stream_id,
>                                    s->streams[stream_id]->codec->codec_type);
> -                ffio_wfourcc(pb, tag);
> +                    ffio_wfourcc(pb, tag);
> +                }
>                  avio_wl32(pb, ie->flags);
>                  avio_wl32(pb, ie->pos);
>                  avio_wl32(pb, ie->len);
> @@ -673,6 +678,7 @@ static int avi_write_packet(AVFormatContext *s, AVPacket *pkt)
>          return avi_write_packet_internal(s, pkt); /* Passthrough */
>  
>      if (enc->codec_type == AVMEDIA_TYPE_VIDEO) {
> +        AVIContext *avi  = s->priv_data;
>          AVIStream *avist = s->streams[stream_index]->priv_data;
>          AVIOContext *pb  = s->pb;
>          AVPacket *opkt   = pkt;
> @@ -709,6 +715,39 @@ static int avi_write_packet(AVFormatContext *s, AVPacket *pkt)
>                      unsigned char tag[5];
>                      avi_stream2fourcc(tag, stream_index, enc->codec_type);
>                      tag[2] = 'p'; tag[3] = 'c';
> +                    if (s->pb->seekable) {
> +                        AVIIndex *idx;
> +                        int cl, id;
> +                        if (avist->strh_flags_offset) {
> +                            int64_t cur_offset = avio_tell(pb);
> +                            avio_seek(pb, avist->strh_flags_offset, SEEK_SET);
> +                            avio_wl32(pb, AVISF_VIDEO_PALCHANGES);
> +                            avio_seek(pb, cur_offset, SEEK_SET);
> +                            avist->strh_flags_offset = 0;
> +                        }
> +                        idx = &avist->indexes;
> +                        cl = idx->entry / AVI_INDEX_CLUSTER_SIZE;
> +                        id = idx->entry % AVI_INDEX_CLUSTER_SIZE;
> +                        if (idx->ents_allocated <= idx->entry) {
> +                            idx->cluster = av_realloc_f(idx->cluster, sizeof(void*), cl+1);
> +                            if (!idx->cluster) {
> +                                idx->ents_allocated = 0;
> +                                idx->entry          = 0;
> +                                return AVERROR(ENOMEM);
> +                            }
> +                            idx->cluster[cl] =
> +                                av_malloc(AVI_INDEX_CLUSTER_SIZE * sizeof(AVIIentry));
> +                            if (!idx->cluster[cl])
> +                                return AVERROR(ENOMEM);
> +                            idx->ents_allocated += AVI_INDEX_CLUSTER_SIZE;
> +                        }

this is identical to code from avi_write_packet_internal()
code should not be duplicated, its a recipe for bugs, duplicated code
will often not be fixed on both sides when one has a bug


> +                        strcpy(idx->cluster[cl][id].tag, tag);

fourccs are not really strings, strcpy is generally a bad idea
security wise


> +                        idx->cluster[cl][id].flags = AVIIF_NO_TIME;
> +                        idx->cluster[cl][id].pos   = avio_tell(pb) - avi->movi_list;
> +                        idx->cluster[cl][id].len   = pal_size * 4 + 4;
> +                        avist->max_size = FFMAX(avist->max_size, pal_size * 4 + 4);
> +                        idx->entry++;

this too is identical to avi_write_packet_internal()


> +                    }
>                      pc_tag = ff_start_tag(pb, tag);
>                      avio_w8(pb, 0);
>                      avio_w8(pb, pal_size & 0xFF);
> @@ -719,13 +758,6 @@ static int avi_write_packet(AVFormatContext *s, AVPacket *pkt)
>                      }
>                      ff_end_tag(pb, pc_tag);
>                      memcpy(avist->old_palette, avist->palette, pal_size * 4);
> -                    if (pb->seekable && avist->strh_flags_offset) {
> -                        int64_t cur_offset = avio_tell(pb);
> -                        avio_seek(pb, avist->strh_flags_offset, SEEK_SET);
> -                        avio_wl32(pb, AVISF_VIDEO_PALCHANGES);
> -                        avio_seek(pb, cur_offset, SEEK_SET);
> -                        avist->strh_flags_offset = 0;
> -                    }
>                  }
>              }
>          }
> diff --git a/tests/ref/lavf-fate/avi_cram b/tests/ref/lavf-fate/avi_cram
> index 7b4e69c..82882fb 100644
> --- a/tests/ref/lavf-fate/avi_cram
> +++ b/tests/ref/lavf-fate/avi_cram
> @@ -1,3 +1,3 @@
> -ba77c5c8bd2b0d1e0478d143346cc3b3 *./tests/data/lavf-fate/lavf.avi
> -928228 ./tests/data/lavf-fate/lavf.avi
> +6fc88702c23b895c305c5e1f51a0904e *./tests/data/lavf-fate/lavf.avi
> +928260 ./tests/data/lavf-fate/lavf.avi
>  ./tests/data/lavf-fate/lavf.avi CRC=0xa4770de2
> -- 
> 1.7.10.4
> 

> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

It is what and why we do it that matters, not just one of them.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160312/ced84f9b/attachment.sig>


More information about the ffmpeg-devel mailing list