[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