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

[Jean Delvare]

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.

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.

-- 
Jean Delvare
SUSE L3 Support