[FFmpeg-devel] [PATCH 2/2] lavf/avienc: Add xxpc entries to index
Mats Peterson
matsp888 at yahoo.com
Sat Mar 12 22:25:27 CET 2016
On 03/12/2016 10:16 PM, Mats Peterson wrote:
> 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
>
HuffYUV is the only fscking codec I know of that has designed its own
spec-breaking BITMAPINFOHEADER. Regarding ASUS, I can accept the fact
that they include the 8 bytes in biSize, but there's the limit.
Mats
More information about the ffmpeg-devel
mailing list