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

[Michael Niedermayer]

On Tue, Feb 01, 2011 at 06:24:45PM +0000, M?ns Rullg?rd wrote:
> "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.

patch welcome

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Dictatorship naturally arises out of democracy, and the most aggravated
form of tyranny and slavery out of the most extreme liberty. -- Plato
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20110201/087ff958/attachment.pgp>