[FFmpeg-devel] [PATCH 2/2] lavf/avienc: Add xxpc entries to index
Mats Peterson
matsp888 at yahoo.com
Sun Mar 13 03:47:53 CET 2016
On 03/12/2016 10:28 PM, Mats Peterson wrote:
> On 03/12/2016 10:25 PM, Mats Peterson wrote:
>> 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
>>
>
> We have "evidence" of three cases where biSize is not 40. That doesn't
> mean it suddenly becomes accepted behaviour to do like that. Merely that
> none of those people who wrote the codecs understood the AVI specs.
>
> Mats
>
Microsoft doesn't mention what to do with non-palette data in a strf
chunk, since there is already a 'strd' chunk directly following the
'strf' chunk for that purpose. Why didn't the authors of those codecs
use that chunk instead of stuffing the data in the 'strf' chunk? Perhaps
the 'strd' chunk was a feature implemented after the codecs were written
like you said, but regardless of that, they shouldn't try and invent
some new variants of BITMAPINFOHEADER, or mess with the biSize value.
Unfortunately, they already have, and those particular codecs have been
around for some time, so adding HuffYUV, ffvhuff, asv1 and asv2 to this
*fixed* list of exceptions is acceptable to me. Adding more entries to
it is not.
If you want to solve this issue in a different way, please go ahead. I
won't change my opinion on this matter.
Mats
More information about the ffmpeg-devel
mailing list