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

[Måns Rullgård]

"Ronald S. Bultje" <rsbultje at gmail.com> writes:

> 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.

Define the struct in some private place and have just a "struct foo *"
in the context.

-- 
M?ns Rullg?rd
mans at mansr.com