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

Jean Delvare jdelvare at suse.de
Wed Dec 16 21:25:17 CET 2015


Hi Derek,

On Wed, 16 Dec 2015 17:16:26 +0000, Derek Buitenhuis 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.

The C spec does say what pointers types are considered the same and
what pointer types should be considered different. Compilers can use
this to warn users about pointer mismatches. That's what
-Wstrict-aliasing is about.

But does the C spec say that compilers should use pointer type
differences to optimize the code by reordering the instructions? I
doubt it. This very much looks like an implementation decision on the
compiler side. That's what -fstrict-aliasing is about.

Obviously both are related as far as the compiler is concerned, as the
pointer type tracking and comparison code has to be common. These are
two different things nevertheless.

> > 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.

Performance, readability, consistency, is what we gain. Apparently you
think they are not worth it, and while I'm surprised, I respect your
decision.

> > 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.

My code is doing the same that is already done everywhere in the ffmpeg
source code tree. It is (maybe) less spec compliant if you look only at
the function I modified. If you look at the project as a whole, it
doesn't make any difference. If such casts were already a problem, then
whoever is affected can already not use ffmpeg, with or without my
patch. Is it the case? Is there any platform where ffmpeg doesn't work
because of this? Any compiler that can't be used to build ffmpeg
because of this?

And this is why I'm surprised, really. There are only two positions
which I can understand. Either these casts are bad, they should be
removed from ffmpeg immediately, and no new occurrence should be
accepted. Or they are not bad, and there should be no objection
introducing new ones. Anything in the middle (as is happening right
now) makes no sense to me.

My own understanding of the situation is that such casts can be
problematic in certain cases [1] and this is why the compilers warn
about them. But the C spec allows for explicit cast between pointer
types for a reason, and that reason is that they are sometimes the
right thing to do. Which I think is the case here.

[1] http://c-faq.com/ansi/constmismatch.html

> 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

You do exactly that as soon as you pass a pointer as a parameter to a
function. So the standard better allows it. The problem isn't with
creating new pointers to existing data, it is creating new pointers of
a different type than the original pointers had.

> [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.

I'm not sure why two, and even if that is the case, I'm not sure what
difference it makes. If both approaches break the standard, I'd go for
the faster or cleaner one, not the one breaking the standard the fewest
times.

> 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'm honestly puzzled. Not to get one of my patches refused, that's part
of the process, it's not the first time and I'm fine with that. But the
whole thing seems to escape logic, twice.

Firstly, the point of -fstrict-aliasing is to optimize the code. But as
it is dangerous, you are advocating that pointers should never be cast
and data should always be copied to temporary buffers instead. The
optimization gain from -fstrict-aliasing would have to be significant
to justify adding extra data copy everywhere. I have a serious doubt
that merely reordering a few instructions can bring in a performance
gain that the extra copies wouldn't at least cancel.

Secondly, the debate here is around const pointers. My original
understanding was that the point of marking function parameters as
const was so that the compiler could warn us if a function was
modifying pointed data that is was only supposed to read. If in fact
this is only a side effect and actually "const" means a completely
different pointer type to the compiler and it uses that for reordering
instructions for optimization purposes, then I'm afraid it is being
misused pretty much everywhere. And if you introduce temporary buffers
to get rid of such warnings, I would question the value of using
"const" in your function parameters in the first place. You'd probably
be better just dropping these const markers so that you can keep your
code clean and fast.

-- 
Jean Delvare
SUSE L3 Support


More information about the ffmpeg-devel mailing list