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

[Hendrik Leppkes]

On Tue, Dec 15, 2015 at 11:20 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
> 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
>

The "speed up" from removing a copy of 4 pointers is negligible as
well however, so maybe it should just be kept like it is.

- Hendrik