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

[Michael Niedermayer]

On Tue, Dec 15, 2015 at 08:58:01AM +0100, Jean Delvare wrote:
> Hallo Michael,
> 
> On Mon, 14 Dec 2015 23:18:39 +0100, Michael Niedermayer wrote:
> > On Mon, Dec 14, 2015 at 07:36:51PM +0100, Jean Delvare wrote:
> > > As I understand it, the temporary stack buffer "src_data" was
> > > introduced solely to avoid a compiler warning. I believe that a better
> > > way to solve this warning it to explicitly cast src->data. This
> > > should be somewhat faster, and just as safe.
> > > 
> > > Signed-off-by: Jean Delvare <jdelvare at suse.de>
> > > Cc: Anton Khirnov <anton at khirnov.net>
> > > ---
> > >  libavutil/frame.c |    4 +---
> > >  1 file changed, 1 insertion(+), 3 deletions(-)
> > > 
> > > --- ffmpeg.orig/libavutil/frame.c	2015-12-14 18:42:06.272234579 +0100
> > > +++ ffmpeg/libavutil/frame.c	2015-12-14 19:05:18.501745387 +0100
> > > @@ -647,7 +647,6 @@ AVFrameSideData *av_frame_get_side_data(
> > >  
> > >  static int frame_copy_video(AVFrame *dst, const AVFrame *src)
> > >  {
> > > -    const uint8_t *src_data[4];
> > >      int i, planes;
> > >  
> > >      if (dst->width  < src->width ||
> > > @@ -659,9 +658,8 @@ static int frame_copy_video(AVFrame *dst
> > >          if (!dst->data[i] || !src->data[i])
> > >              return AVERROR(EINVAL);
> > >  
> > > -    memcpy(src_data, src->data, sizeof(src_data));
> > >      av_image_copy(dst->data, dst->linesize,
> > > -                  src_data, src->linesize,
> > > +                  (const uint8_t **)src->data, src->linesize,
> > 
> > I think this may be a aliasing violation and thus undefined
> > not that i like that or consider that sane
> > so if someone says iam wrong, i would certainly not be unhappy
> 
> Why would it be an aliasing violation? We do not change the fundamental
> type of the pointer, we only add a const where the function wants it.
> For more simple pointer types the compiler would do it for us silently.

A. uint8_t and const uint8_t are distinct types
        "Any type so far mentioned is an unqualified type. Each unqualified type has several
        qualified versions of its type,38) corresponding to the combinations of one, two, or all
        three of the const, volatile, and restrict qualifiers. The qualified or unqualified
        versions of a type are distinct types that belong to the same type category and have the
        same representation and alignment requirements."
B. a pointer to uint8_t and const uint8_t are not qualified types of the same type
   See the example in 6.2.5
        "28 EXAMPLE 1 The type designated as ‘‘float *’’ has type ‘‘pointer to float’’. Its type category is
        pointer, not a floating type. The const-qualified version of this type is designated as ‘‘float * const’’
        whereas the type designated as ‘‘const float *’’ is not a qualified type — its type is ‘‘pointer to constqualified
        float’’ and is a pointer to a qualified type."
C. They are not compatible
        "Two types have compatible type if their types are the same. Additional rules for
        determining whether two types are compatible are described in 6.7.2 for type specifiers,
        in 6.7.3 for type qualifiers, and in 6.7.5 for declarators.46)"
D. None of the aliasing exceptions seems to apply
        "An object shall have its stored value accessed only by an lvalue expression that has one of the following types:76)
        — atype compatible with the effective type of the object,
        — aqualified version of a type compatible with the effective type of the object,"
(its not a qualified version)
       " — a type that is the signed or unsigned type corresponding to the effective type of the object,
        — a type that is the signed or unsigned type corresponding to a qualified version of the effective type of the object,
        — an aggregate or union type that includes one of the aforementioned types among its members (including, recursively, a member of a subaggregate or contained union), or
        — acharacter type."
(dont apply)

also:
        "For two qualified types to be compatible, both shall have the identically qualified version
        of a compatible type; the order of type qualifiers within a list of specifiers or qualifiers
        does not affect the specified type."

        "For two pointer types to be compatible, both shall be identically qualified and both shall
        be pointers to compatible types."

I quite possible could misinterpret or miss some parts though ...


> 
> Also I can see 12 occurrences of the same cast for this parameter of
> function av_image_copy() in the ffmpeg code already. And over 20 more
> similar casts for similar parameters of other functions
> (ff_combine_frame, swr_convert, copy_picture_field...) So I'm not
> introducing anything new, just proposing one more of the same.

yes, I have no real oppinion on this except that C is insane or I am
and i dont really mind to apply the patch if thats what people prefer.
Any real compiler that follows this litterally and breaks the code is
IMHO a compiler one should avoid (quite independant of it being used
for FFmpeg or other things) unless one wants to language lawyer around
on such things instead of writing usefull code

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

It is what and why we do it that matters, not just one of them.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20151215/99e1c816/attachment.sig>