Quoting Vittorio Giovara (2016-08-24 16:58:12)
> On Wed, Aug 24, 2016 at 6:06 AM, Diego Biurrun <[email protected]> wrote:
> > On Wed, Aug 24, 2016 at 11:24:01AM +0200, Anton Khirnov wrote:
> >> --- /dev/null
> >> +++ b/libavutil/x86/imgutils_init.c
> >> @@ -0,0 +1,49 @@
> >> +
> >> +void ff_image_copy_plane_uc_from_sse4(uint8_t *dst, ptrdiff_t
> >> dst_linesize,
> >> + const uint8_t *src, ptrdiff_t
> >> src_linesize,
> >> + ptrdiff_t bytewidth, int height);
> >> +
> >> +int ff_image_copy_plane_uc_from_x86(uint8_t *dst, ptrdiff_t
> >> dst_linesize,
> >> + const uint8_t *src, ptrdiff_t
> >> src_linesize,
> >> + ptrdiff_t bytewidth, int height)
> >> +{
> >> + int cpu_flags = av_get_cpu_flags();
> >> + ptrdiff_t bw_aligned = FFALIGN(bytewidth, 64);
> >> +
> >> + if (EXTERNAL_SSE4(cpu_flags) &&
> >> + bw_aligned <= dst_linesize && bw_aligned <= src_linesize)
> >> + ff_image_copy_plane_uc_from_sse4(dst, dst_linesize, src,
> >> src_linesize,
> >> + bw_aligned, height);
> >> + else
> >> + return AVERROR(ENOSYS);
> >> +
> >> + return 0;
> >> +}
> >> --- a/libavutil/imgutils.c
> >> +++ b/libavutil/imgutils.c
> >> @@ -265,9 +266,33 @@ void av_image_copy_plane(uint8_t *dst, int
> >> dst_linesize,
> >> +static void image_copy_plane_uc_from(uint8_t *dst, ptrdiff_t
> >> dst_linesize,
> >> + const uint8_t *src, ptrdiff_t
> >> src_linesize,
> >> + ptrdiff_t bytewidth, int height)
> >> +{
> >> + int ret = -1;
> >> +
> >> +#if ARCH_X86
> >> + ret = ff_image_copy_plane_uc_from_x86(dst, dst_linesize, src,
> >> src_linesize,
> >> + bytewidth, height);
> >> +#endif
> >> +
> >> + if (ret < 0)
> >> + image_copy_plane(dst, dst_linesize, src, src_linesize, bytewidth,
> >> height);
> >> +}
> >
> > This feels awkward. Why not make it into some dsp-like structure with
> > overloading function pointers?
>
> +1
How is this more awkward than doing almost the exact same thing in the dsp
context init function?
My first approach actually did add a new dsp context, but it turned out
that it's just a lot of extra boilerplat code for no real reason.
>
> >> @@ -265,9 +266,33 @@ void av_image_copy_plane(uint8_t *dst, int
> >> dst_linesize,
> >>
> >> -void av_image_copy(uint8_t *dst_data[4], int dst_linesizes[4],
> >> - const uint8_t *src_data[4], const int src_linesizes[4],
> >> - enum AVPixelFormat pix_fmt, int width, int height)
> >> @@ -289,17 +314,41 @@ void av_image_copy(uint8_t *dst_data[4], int
> >> dst_linesizes[4],
> >>
> >> +void av_image_copy(uint8_t *dst_data[4], int dst_linesizes[4],
> >> + const uint8_t *src_data[4], const int src_linesizes[4],
> >> + enum AVPixelFormat pix_fmt, int width, int height)
> >> +{
> >> + ptrdiff_t dst_linesizes1[4], src_linesizes1[4];
> >> + int i;
> >> +
> >> + for (i = 0; i < 4; i++) {
> >> + dst_linesizes1[i] = dst_linesizes[i];
> >> + src_linesizes1[i] = src_linesizes[i];
> >> + }
> >> +
> >> + image_copy(dst_data, dst_linesizes1, src_data, src_linesizes1,
> >> pix_fmt,
> >> + width, height, image_copy_plane);
> >> +}
> >
> > I think it's time to deprecate the int types for the line sizes and use
> > ptrdiff_t everywhere instead.
>
> +1
And? I do not see what does that have to do with this patch.
>
> also, does it have to be a separate function? If so, what does the old
> function do that the new one does not?
The doxy says what the difference is -- mainly that this function requires
alignment.
We could conceivably add some checks that make it fall back on the
normal memcpy for unaligned data, but I'm not convinced that using this
special copy for normal frames is a good idea.
--
Anton Khirnov
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
