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

[Michael Niedermayer]

On Tue, Sep 07, 2010 at 10:00:20PM +0200, Stefano Sabatini wrote:
> On date Sunday 2010-09-05 23:24:56 +0200, Michael Niedermayer encoded:
> > On Sun, Sep 05, 2010 at 08:59:17PM +0200, Stefano Sabatini wrote:
> > > On date Sunday 2010-09-05 18:59:34 +0200, Michael Niedermayer encoded:
> > > > On Sat, Sep 04, 2010 at 02:59:50PM +0200, Stefano Sabatini wrote:
> > > > > On date Saturday 2010-09-04 13:45:30 +0200, Michael Niedermayer encoded:
> > > > > > On Sat, Sep 04, 2010 at 10:52:08AM +0200, Stefano Sabatini wrote:
> > > > > > > On date Saturday 2010-09-04 01:27:41 +0200, Michael Niedermayer encoded:
> > > > > [...]
> > > > > > > > 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.
> > > > > > 
> > > > > > the doxy is poor, the name is poor
> > > > > > call it av_image_copy_plane() for example
> > > > > > 
> > > > > > and doxy could be
> > > > > > 
> > > > > > Copy image plane from src to dst.
> > > > > > That is, copy "height" number of lines of "bytewidth" bytes each.
> > > > > > The first byte of each successive line is seperated by *_linesize bytes.
> > > > > 
> > > > > Doxy fixed.
> > > > > 
> > > > > As for the name I already explained that imgutils.h API is not using a
> > > > > hierachical naming scheme, so av_copy_image_plane() is consistent with
> > > > > the rest of the API (and imho more resembling the English language ->
> > > > > more readable).
> > > > 
> > > > and i prefer an hierachical naming sheme here, its not a strong preferance
> > > > but it has its nice features like you can grep for things easier
> > > 
> > > av_.*image works as well.
> > 
> > not here:
> > 
> > libavcodec/flashsv.c:    av_log(avctx, AV_LOG_DEBUG, "image: %dx%d block: %dx%d num: %dx%d part: %dx%d\n",
> > libavcodec/flashsvenc.c:    s->encbuffer = av_mallocz(s->image_width*s->image_height*3);
> > libavcodec/flashsvenc.c:        s->previous_frame = av_mallocz(FFABS(p->linesize[0])*s->image_height);
> > libavcodec/flashsvenc.c:        av_log(avctx, AV_LOG_ERROR, "buf_size %d <  %d\n", buf_size, s->image_width*s->image_height*3);
> > 
> > 
> > > 
> > > My other concerns is consistency, sorry to bother but I'd hate to have
> > > a mixed style API.
> > 
> > IIRC the api was consistently using hierachical naming before you
> > moved it to libavutil
> 
> Updated.
> -- 
> FFmpeg = Fantastic Furious Mysterious Pitiless Everlasting Guide

>  libavcodec/dsputil.c    |    3 ++-
>  libavcodec/dsputil.h    |    7 +++++++
>  libavcodec/imgconvert.c |   14 ++++----------
>  libavcore/imgutils.c    |   13 +++++++++++++
>  libavcore/imgutils.h    |   13 +++++++++++++
>  5 files changed, 39 insertions(+), 11 deletions(-)
> c3bb2ad374e2817487b221532a03c89a00bb27d1  0001-Reimplement-ff_img_copy_plane-as-av_image_copy_plane.patch

should be ok

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

Freedom in capitalist society always remains about the same as it was in
ancient Greek republics: Freedom for slave owners. -- Vladimir Lenin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100907/c63e4069/attachment.pgp>