[FFmpeg-devel] [PATCH] Make av_set_pts_info keep previous time base if new one is invalid.

[Ronald S. Bultje]

Hi,

On Sun, Feb 6, 2011 at 2:29 PM, Reimar D?ffinger
<Reimar.Doeffinger at gmx.de> wrote:
> On Sun, Feb 06, 2011 at 12:35:04PM -0500, Ronald S. Bultje wrote:
>> On Sun, Feb 6, 2011 at 12:00 PM, Reimar D?ffinger
>> <Reimar.Doeffinger at gmx.de> wrote:
>> > On 6 Feb 2011, at 16:05, Michael Niedermayer <michaelni at gmx.at> wrote:
>> >> On Sun, Feb 06, 2011 at 03:20:37PM +0100, Reimar D?ffinger wrote:
>> >>> On Sun, Feb 06, 2011 at 02:36:28PM +0100, Michael Niedermayer wrote:
>> >>>> On Sun, Feb 06, 2011 at 11:37:52AM +0100, Reimar D?ffinger wrote:
>> >>>>> Can be done, even though I'd like to note that I don't see why it has
>> >>>>> to be in one patch, the things you mention were an issue before,
>> >>>>> this fixes just one issue: setting values that make the rest of
>> >>>>> the code crash.
>> >>>>
>> >>>> I never said it has to be in one patch.
>> >>>
>> >>> I'm really not in the mood for these crappy reviews.
>> >>
>> >> what is it that ive now said to upset you?
>> >
>> > It's not just you, but all the time, almost no matter how serious the issue is people come and dump all kinds of barely related blue-sky wishes.
>> > I have then the choice of either implementing them even if I think they are just a waste of time and hope that those people didn't forget to review the patch itself (since there's no clear statement about that) and then reject it flat out after all the extra effort, or I start discussing each point, which then often enough results in no reply at all anymore and the patch being dead ?(even if it's just until the next day that I get an answer that sucks if it is _today_ that I have time for it).
>>
>> That's unfair, I asked for a return value at some point in the future,
>> I didn't say I wouldn't accept the current patch as-is.
>
> IIRC you didn't say you would either. Exactly that is my complaint: not saying
> anything what a patch submitter cares more about: how do I get the patch in?

OK, I'll be clearer in the future. Patch queued for now, it fixes an
obvious issue.

>> A return value is useful, so that callers can detect failures to set a
>> timebase, and error out in read_header() instead of parsing garbage.
>> If you know of any use case where this doesn't apply (i.e. the output
>> is not garbage), I'd love to hear it.
>
> A single-bit error in the time base? Highly unlikely normally but
> I don't see what you would win by refusing a file because of a hint
> there might be only garbage instead of continuing until you actually know.
> And I am not saying that a return value is certainly useless, but I
> dislike uglifying the API for "might-be".
> If it wasn't a public API function I'd have added a return value right
> away.

I'll leave it as-is for now, if others agree
a-return-value-so-demuxers-can-error-out would be nice, I can pick it
up from here and introduce av_set_pts_info2() or so.

Ronald