[FFmpeg-devel] [PATCH 2/2] lavf/avienc: Add xxpc entries to index
Mats Peterson
matsp888 at yahoo.com
Sat Mar 12 16:37:39 CET 2016
On 03/12/2016 04:33 PM, Mats Peterson wrote:
> On 03/12/2016 04:29 PM, Michael Niedermayer wrote:
>> On Sat, Mar 12, 2016 at 02:54:45PM +0100, Mats Peterson wrote:
>>> On 03/12/2016 02:52 PM, Michael Niedermayer wrote:
>>>> On Sat, Mar 12, 2016 at 02:36:59PM +0100, Mats Peterson wrote:
>>>>> On 03/12/2016 02:26 PM, Mats Peterson wrote:
>>>>>> 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.
>>>>>
>>>>>
>>>>> It's not really uninitalised, is it? Since it isn't used by anything
>>>>> but my own code, it's all zero bytes, right?
>>>>
>>>> after this patch snow encoding failed, i run valgrind and saw this
>>>> so there was something wrong, i dont know for sure if it was this
>>>>
>>>>
>>>
>>> OK. By the way, are you in the process of applying patch 1 at least?
>>
>> patch one has a list of exceptions, this list contains every single
>> case for which evidence has been provided
>> that is all evidence provided so far is consistent in that a biSize
>> of 40 is wrong for non palette global headers.
>
> 40 is exactly what it should be, in *all* cases, regardless of what
> follows the BITMAPINFOHEADER. Read below.
>
>>
>> The spec says:
>> "A stream format chunk ('strf') must follow the stream header chunk.
>> The stream format chunk describes the format of the data in the
>> stream. The data contained in this chunk depends on the stream type.
>> For video streams, the information is a BITMAPINFO structure,
>> including palette information if appropriate. For audio streams, the
>> information is a WAVEFORMATEX structure."
>>
>> If that chunk is a BITMAPINFO structure + a palette then formats
>> without a palette would likely have biSize similar to the chunk
>> size ...
>>
>> its quite possible iam missing some details of course ...
>>
>
> You're missing the specification of the BITMAPINFOHEADER. It states that
> biSize is the size of the structure, i.e. the BITMAPINFOHEADER itself.
> No less, no more.
>
> https://msdn.microsoft.com/en-us/library/windows/desktop/dd183376%28v=vs.85%29.aspx
>
>
Now, the HuffYUV author has invented his own spec-breaking
BITMAPINFOHEADER, and for asv1/asv2, I can accept a biSize of 48 since
that's what ASUS writes to their files. But that's it.
Mats
More information about the ffmpeg-devel
mailing list