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

Mats Peterson matsp888 at yahoo.com
Sat Mar 12 14:26:00 CET 2016


On 03/12/2016 12:53 PM, Michael Niedermayer wrote:
> 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
>

OK.

>
>> +    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)
>

OK.

>
>
>> +                    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
>

In my book, any code is duplicated code, depending on how you look at 
it. Do you have a better suggestion? Shared static function, perhaps?

>
>> +                        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
>
>
>
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>


-- 
Mats Peterson
http://matsp888.no-ip.org/~mats/


More information about the ffmpeg-devel mailing list