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

[Derek Buitenhuis]

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.

- Derek