[FFmpeg-devel] [PATCH] lavu/frame: Optimize frame_copy_video

[Paul B Mahol]

On 12/16/15, Derek Buitenhuis <derek.buitenhuis at gmail.com> wrote:
> On 12/15/2015 10:39 PM, Jean Delvare wrote:
>> I see two completely different things.
>>
>> The change I proposed is specific to one function, only changes const
>> vs non-const, and the object under discussion is being accessed for
>> reading only (thus the const pointer.) It would remove a memcpy but
>> does not introduce a direct copy (with =.)
>
> See below.
>
>> The change you pointed us to is touching very generic functions,
>> handling by nature a very wide variety of pointer types for both
>> reading and writing.
>>
>> So just because the change you pointed us to may be good and desirable,
>> doesn't imply that the change I am proposing is bad and undesirable.
>>
>> Side note #1: if gcc really considers pointer types that only differ by
>> "const" as different when it comes to code optimization, it is
>> seriously broken IMHO. Unfortunately the man page is quite vague on the
>> topic ("unless the types are almost the same", can you be more vague
>> please.)
>
> The C spec does. GCC happens to handle it correctly. We support a whole lot
> more compilers than GCC.
>
>> Side note #2: if strict-aliasing optimizations can do so much damage
>> to your code that you end up doing
>>   memcpy(arg, &(void *){ NULL }, sizeof(val));
>> to set a pointer to NULL, maybe you should consider building with
>> -fno-strict-aliasing.
>
>
> Again, that is GCC-specific. My point is that if we *can* be correct, with
> regards to the spec, we should be. We don't gain anything, IMO, by removing
> this particular thing.
>
>>
>> Side note #3: I'm surprised this change was needed in the first place
>> as my understanding was that gcc would skip all strict-aliasing
>> optimizations for void pointers, which is what the affected functions
>> are handling. It's sad that the commit message is as vague as gcc's
>> manual page on the topic.
>
> If a change makes some code more spec compliant, with little to no downside,
> I fail to see why it shouldn't be incldued.
>
> Your patch here makes code *less* spec compliant for little to no gain.
>
> P.S. It actually looks like the original code is not entirely compliant,
> after
> discussing with some people Smarter Than Me:
>
> [17:06] <+courmisch> memory copying the pointer fixes the aliasing problem
> on pointer itself
> [17:06] <+courmisch> but I suspect it leaves an aliasing problem with the
> pointed data
> [17:07] <+courmisch> specifically, I am not sure the standard allows
> creating pointers to existing data out of thin air
> [17:07] <+courmisch> which memcpy() effectively does if it changes the
> pointer type
>
> So, as a I see it, this patch replaced one aliasing violation with two.
>
> If an argument can be made that there is a measurable speedup, and none of
> our supported
> compilers abuse the C spec, as compilers are wont to do, then perhaps it's
> worth revisiting,
> but otherwise, I'd prefer to NAK it. I'm obviously not The Final Word here,
> but this is
> my 2 cents.

I really do not see gain in this patch so NAK from me too.