[FFmpeg-devel] [PATCH 1/1] Reimplement ff_img_copy_plane() as av_img_copy_plane() in libavcore, and deprecate the old function.

[Stefano Sabatini]

On date Saturday 2010-09-04 01:27:41 +0200, Michael Niedermayer encoded:
> On Fri, Aug 27, 2010 at 01:47:26AM +0200, Stefano Sabatini wrote:
> > On date Thursday 2010-08-26 21:40:32 +0200, Michael Niedermayer encoded:
> > > On Thu, Aug 26, 2010 at 02:27:27AM +0200, Stefano Sabatini wrote:
> > > > On date Thursday 2010-08-26 00:44:12 +0200, Michael Niedermayer encoded:
> > > [...]
> > > 
> > > > > if nothing outside needs it then no need to make it public and as is
> > > > > the param should be renamed
> > > 
> > > after your patch it definitly is used outside.
> > > 
> > > 
> > > > 
> > > > Updated, regards.
> > > 
> > > 
> > > [...]
> > > > --- a/libavcodec/dsputil.c
> > > > +++ b/libavcodec/dsputil.c
> > > > @@ -27,6 +27,7 @@
> > > >   * DSP utils
> > > >   */
> > > >  
> > > > +#include "libavcore/internal.h"
> > > >  #include "avcodec.h"
> > > >  #include "dsputil.h"
> > > >  #include "simple_idct.h"
> > > > @@ -4466,7 +4467,7 @@ av_cold void dsputil_init(DSPContext* c, AVCodecContext *avctx)
> > > >      c->sv_fmul_scalar[0] = sv_fmul_scalar_2_c;
> > > >      c->sv_fmul_scalar[1] = sv_fmul_scalar_4_c;
> > > >  
> > > > -    c->shrink[0]= ff_img_copy_plane;
> > > > +    c->shrink[0]= ff_copy_image_plane;
> > > [...]
> > > > --- a/libavcore/imgutils.c
> > > > +++ b/libavcore/imgutils.c
> > > > @@ -21,6 +21,7 @@
> > > >   * misc image utilities
> > > >   */
> > > >  
> > > > +#include "internal.h"
> > > >  #include "imgutils.h"
> > > >  #include "libavutil/pixdesc.h"
> > > >  
> > > > @@ -120,3 +121,16 @@ int av_check_image_size(unsigned int w, unsigned int h, int log_offset, void *lo
> > > >      av_log(&imgutils, AV_LOG_ERROR, "Picture size %ux%u is invalid\n", w, h);
> > > >      return AVERROR(EINVAL);
> > > >  }
> > > > +
> > > > +void ff_copy_image_plane(uint8_t       *dst, int dst_linesize,
> > > > +                         const uint8_t *src, int src_linesize,
> > > > +                         int bytewidth, int height)
> > > > +{
> > > > +    if (!dst || !src)
> > 
> > Yes but this is an internal dependancy, so no exposed to the general
> > public, that was what I meant since the beginning.
> 
> if its used by a differnt lib then it is external and part of the ABI/API
> it also then requires all the versioning, major and minor and soname bumping
> if it changes. Thus the API has to be reviewed more throughout than if its
> just internal where we can change it trivially

So tell what's wrong with the following and possibly suggest what you
prefer instead, thus avoiding guessloops to me.

/**
 * Copy image plane data in src to dst.
 *
 * @param dst_linesize linesize for the image plane in dst
 * @param src_linesize linesize for the image plane in src
 * @param bytewidth the width in bytes of the image
 * @param height the height of the image
 */
void ff_copy_image_plane(uint8_t       *dst, int dst_linesize,
                         const uint8_t *src, int src_linesize,
                         int bytewidth, int height);

Currently (with the latest patch) this function is used in
dsputil_init() and in av_picture_data_copy().

bytewidth can be obtained from width using the function
av_get_image_linesize().

A variant may take in input a width rather than a bytewidth, but this
would require to create a wrapper for dsputil_init() which expect a
function with the current behavior.

Regards.
-- 
FFmpeg = Frenzy and Funny Miracolous Portentous Elitist Gospel