[FFmpeg-devel] [PATCH] seek print NOPTS

[Baptiste Coudurier]

On 10/20/2009 01:08 PM, Reimar D?ffinger wrote:
> On Tue, Oct 20, 2009 at 11:55:43AM -0700, Baptiste Coudurier wrote:
>> On 10/20/2009 11:24 AM, Reimar D?ffinger wrote:
>>>> Now changing to NOPTS is ok for cosmetics reason ? And you guys
>>>> couldn't suggest that in the first place ?
>>>
>>> Michael did suggest something like I ended up doing in one
>>
>> The problem is that fixing windows regression tests is IMHO far more
>> important than this cosmetic change, however in the past thread,
>> IMHO it seems that instead of working together in the best direction
>> for the code itself, people ended up rejecting it in a unreasonable
>> manner.
>
> I was starting to write something else, but I decided to just try to
> summarize the events, because this gives me a completely different view
> of the real "issue" here.
> Because from a technical point I don't think it could have been handled any
> better, I do wonder if you see it differently or if your complaints
> are "_only_" about the style/terseness/friendlyness of the responses,
> in which case I very much misunderstood what you wrote so far.

My complaints are mostly about the style, indeed.

> The first patch on May 1st said:
>> Attached patch makes seek_test print AV_NOPTS_VALUE instead of some big
>> negative and apparently meaningless number.
>
> To which the response was around May 2nd:
>> Complicating the code for no apparent reason, the regression tests are
>> just for developers knowing what they do. And someone not knowing what
>> the big meaningless number is wont be able to make any sense of
>> "AV_NOPTS_VALUE" either.
>
> While I don't 100% agree, it seems not completely unreasonable to me,
> and for the amount of code (duplication) I think the original mail
> did indeed give insufficient reason. Agreed or not?

Well, IMHO printing "NOPTS" instead of "AV_NOPTS_VALUE" is not much 
different. So I'd say the idea is the same, and is good.

Therefore I would have more expected Michael to say the same thing 
regarding your patch. Now, like I said in my first mail in this thread, 
it didn't go the same way, and I'm wondering why :)

Now the first patch was complicating the code. I can understand the 
refusal, but usually when idea is good but badly implemented, reviews 
tend to say: "you can simplify that".

> Shortly after, a more thorough explanation was given:
>> Seek test fail on MinGW because MSVCRT doesn't properly printf() big
>> numbers. Instead of printing
>> pts:-102481911520608.625000
>> it prints
>> pts:-102481911520608.600000
>
> One response to that being
>> So fix it.
>
> With "it" referring to printf. Ignoring the tone, I do think it rightfully
> points out that we have not been given a reason why this can't be fixed
> "at the source", which is what I think we agree should generally be done
> (even if at least I am sometimes too lazy for it).

Yes, no doubt about it, fixing the source is better, when the source is 
willing to do it in a reasonable timeframe, which is not necessarly the 
case but was in this one.

I'm tempted to think that IMHO if the mingw reason was not given, the 
patch would have still been perfectly valid although a matter of 
personal taste.

> Then there was the -1 and NaN suggestions which did not work out.
>  From a technical standpoint again I think there was no reasonable
> way to know in advance those would be a waste of time and I do
> think they were reasonable to ask to investigate since it was not
> that much effort to do.
> Then the whole effort died, resurrected only by your ping on the 26th
> September, which resulted in a more specific suggestion for a fix and
> realizing that it was fixed from the MinGW side, which lead the whole
> thing to be dropped again.
> Here the only issue I can see is the long delay.
> For what I can see that was caused by at least one of
> 1) the issue being fixed by MinGW
> 2) nobody caring about Windows/MinGW
>
> As for 1), that surely is not really an issue, even though it would
> have been nicer to know about it earlier, right?

Right.

> As for 2), well, I'm tempted to say that that's just how it is - I'd welcome
> solutions, but you can not force Linux developers care about it to the point
> of wildly trying to create patches they can't test nor does there seem to
> be much of another way to get more active Windows developers judging
> by the lack of success by the VLC team.

Well, it seems some people care for obscure architectures FATE tests at 
least as much as windows. Of course I cannot force anybody to care, but 
since windows is on FATE, it must be taken care of, maybe not as much as 
Linux but as much as OSX x86 IMHO.

> Though I can suggest having a look at MPlayer's DOCS/tech/mingw-crosscompile.txt
> which explains how to do a cross-compile for MinGW under Debian - if
> someone modifies this to work with FFmpeg and maybe adds whatever is
> necessary so make test can run the result with Wine, maybe we can
> get a few (more) Linux developers to take test/care about Windows-specific
> patches?
> Any other specific issue you see that I missed or suggestions how to fix it?

Not really, I'm fine with the patch as I always was.

Now imagine if answers to Ramiro's first mail were:
"code is too complicated, please simplify, though the idea is ok, it's 
definitely better and nicer to have NOPTS instead of this big number, 
furthermore text will be nicely aligned."

Wouldn't this be more efficient and nicer for everybody ?

-- 
Baptiste COUDURIER
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
FFmpeg maintainer                                  http://www.ffmpeg.org