[FFmpeg-devel] [PATCH 2/2] lavf/avienc: Add xxpc entries to index
Mats Peterson
matsp888 at yahoo.com
Sat Mar 12 22:16:02 CET 2016
On 03/12/2016 06:39 PM, Michael Niedermayer wrote:
> On Sat, Mar 12, 2016 at 04:37:39PM +0100, Mats Peterson wrote:
>> 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.
>
> its not neccessary for you or me to accept either case.
> its neccessary to provide evidence
>
> ATM there is evidence for 3 cases that have biSize include the non
> palette extradata and 0 cases that do not.
>
In this case it's rather a sign of not understanding or not following
the AVI specs rather than evidence.
> the spec suggests that strd not strf should be used
> neither code before nor after the patch does that, nor do actual
> official codecs i would suspect. maybe because they predate strd,
> its not part of docuemnts from 1997 that i have laying around.
Could be the case, yes. And I agree that codecs should use strd, since
it's designed for that purpose.
>
> Thus i suspect (but do not know) that redefining BITMAPINFOHEADER was
> neccessary back then to store a global header, which is likely what
> everyone did. I wouldnt call that spec-breaking
>
You wouldn't call using any value other than 40 for biSize
spec-breaking? I thought you cared more about following specs than that.
Just because you have extra data following the BITMAPINFOHEADER, you
don't need a different value for biSize. Just subtract 40 from the strf
chunk size. Once again, and I don't know how many times I'll have to
repeat this, biSize is the value of the BITMAPINFOHEADER WITHOUT ANY
EXTRA DATA.
Mats
More information about the ffmpeg-devel
mailing list