Re: [PATCH] MNG detection fails with libjpeg-turbo

The Wanderer <[email protected]> Fri, 01 Dec 2017 19:23:50 -0500
Newsgroups	gmane.comp.video.mplayer.devel
Organization	This space intentionally left blank.
Message-ID	<[email protected]>
On 2017-12-01 at 18:11, Alexander Strasser wrote:

> On 2017-11-30 23:21 -0500, The Wanderer wrote:
> 
>> On 2017-11-30 at 16:23, Alexander Strasser wrote:

>>> Would you be OK with omitting the function defined in the
>>> previous hunk and changing this to use statement_check_broken ?
>> 
>> I would have no objection to it, as long as we understand what
>> it's doing and what the consequences will be.
>> 
>> My primary concern is: why was this put there in the first place? 
>> Presumably it served some purpose at some point...
> 
> I did some digging... it was like that since it was added in 2008.
> The configure check for libmng was initially added for the mng
> demuxer.
> 
> Then I looked at the actual mail thread, where the patch was
> discussed. It was first submitted with a configrue check that
> executed the compiled binary. That was questioned and later deemed
> not acceptable because it would always make the test fail when
> cross-compiling:
> 
> http://mplayerhq.hu/pipermail/mplayer-dev-eng/2008-September/058673.html

I haven't taken the time to read the discussion now, but based on that
description, I think I may even vaguely remember its occurring.

>> I'd be leery of dropping code whose intended purpose we don't know
>> or understand, but if it works equally well without that and we
>> make sure that our own reasoning is findable in case someone does
>> turn up a problem which could be the reason later on, then I have
>> no strong objection.
> 
> With above explanation added to the commit message, that would
> explain our reasoning, right?

It would be even better than what I'd intended, which was just to
explain "since this is never run, we don't see how it can be useful, so
we're dropping it rather than make other parts of the code more complex;
however, we don't know why this was added, so this may need to be
revisited in the future".

Since you've tracked down the reason why it was added, no such "we don't
know" caveat is needed, and I don't see any reason not to drop that
return statement. IMO it should probably have been removed along with
the removal of "execute the compile-check binary".

> After doing some code archaeology, I still intent to apply my local 
> version.

I approve this.

-- 
   The Wanderer

The reasonable man adapts himself to the world; the unreasonable one
persists in trying to adapt the world to himself. Therefore all
progress depends on the unreasonable man.         -- George Bernard Shaw

_______________________________________________
MPlayer-dev-eng mailing list
[email protected]
https://lists.mplayerhq.hu/mailman/listinfo/mplayer-dev-eng
signature.asc (application/pgp-signature, 833 B)