[FFmpeg-devel] [PATCH] lavf: accept time base from untrusted codecs if it matches timings

[Ronald S. Bultje]

Hi,

On Tue, Feb 1, 2011 at 12:17 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Tue, Feb 01, 2011 at 12:15:34PM -0500, Ronald S. Bultje wrote:
>> Hi,
>>
>> On Tue, Feb 1, 2011 at 11:49 AM, Anssi Hannula <anssi.hannula at iki.fi> wrote:
>> > On 01.02.2011 18:10, Ronald S. Bultje wrote:
>> >> On Sun, Jan 30, 2011 at 2:33 PM, Anssi Hannula <anssi.hannula at iki.fi> wrote:
>> >>> @@ -627,6 +627,8 @@ typedef struct AVStream {
>> >>> ? ? ? ? int64_t last_dts;
>> >>> ? ? ? ? int64_t duration_gcd;
>> >>> ? ? ? ? int duration_count;
>> >>> + ? ? ? ?int codec_tb_matches_dts;
>> >>> + ? ? ? ?double codec_tb_dur_error;
>> >>> ? ? ? ? double duration_error[MAX_STD_TIMEBASES];
>> >>> ? ? ? ? int64_t codec_info_duration;
>> >>> ? ? } *info;
>> >>
>> >> These are only used in av_find_stream_info(), adding them to AVStream
>> >> in the middle breaks ABI and increases size for something that I don't
>> >> think should be in there. Can you locally allocate and free an array
>> >> instead, or create a AVFindStreamInfo array for these kind of temp
>> >> values to be shared between functions?
>> >
>> > They are not directly in AVStream and do not increase the size of
>> > AVStream nor change the API.
>> >
>> > Here's a longer excerpt from the above section (before the patch):
>> >
>> > ? ?/**
>> > ? ? * Stream informations used internally by av_find_stream_info()
>> > ? ? */
>> > #define MAX_STD_TIMEBASES (60*12+5)
>> > ? ?struct {
>> > ? ? ? ?int64_t last_dts;
>> > ? ? ? ?int64_t duration_gcd;
>> > ? ? ? ?int duration_count;
>> > ? ? ? ?double duration_error[MAX_STD_TIMEBASES];
>> > ? ? ? ?int64_t codec_info_duration;
>> > ? ?} *info;
>>
>> Oh right it's a pointer. I still consider it ugly, ohwell if that's
>> how it works now then I guess it's OK.
>
> so how would you do it non ugly?
>
> This kind of comments remind me of mplayer, people also said "ugg ugly"
> then fixed it to be pretty but it stoped working bcause the ugly was
> actually the cleanest way that worked people just didnt understand it

I didn't say I had a non-ugly way, I just said it was ugly to put
obviously private members in a publicly exposed struct, not even under
a HAVE_AV_CONFIG_H (so that the thing is allocated internally at the
right size, and anything at the end "appended" to the publicly exposed
API is considered private/internal) or something alike.

It's just asking for trouble.

But like I said, it's fine, I won't fight this fight yet.

Ronald