[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 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

or if you look at existing apis in libavutil
void av_md5_init(struct AVMD5 *ctx);
void av_md5_update(struct AVMD5 *ctx, const uint8_t *src, const int len);
void av_md5_final(struct AVMD5 *ctx, uint8_t *dst);
void av_md5_sum(uint8_t *dst, const uint8_t *src, const int len);

void av_sha1_init(struct AVSHA1* context);
void av_sha1_update(struct AVSHA1* context, const uint8_t* data, unsigned int len);

void av_sha1_final(struct AVSHA1* context, uint8_t digest[20]);

void *av_tree_find(const struct AVTreeNode *root, void *key, int (*cmp)(void *key, const void *b), void *next[2]);
void *av_tree_insert(struct AVTreeNode **rootp, void *key, int (*cmp)(void *key, const void *b), struct AVTreeNode **next);
void av_tree_destroy(struct AVTreeNode *t);

void av_fifo_free(AVFifoBuffer *f);
void av_fifo_reset(AVFifoBuffer *f);

int av_des_init(struct AVDES *d, const uint8_t *key, int key_bits, int decrypt);
void av_des_crypt(struct AVDES *d, uint8_t *dst, const uint8_t *src, int count, uint8_t *iv, int decrypt);

int av_crc_init(AVCRC *ctx, int le, int bits, uint32_t poly, int ctx_size);
const AVCRC *av_crc_get_table(AVCRCId crc_id);
uint32_t av_crc(const AVCRC *ctx, uint32_t start_crc, const uint8_t *buffer, size_t length) av_pure;

int av_base64_decode(uint8_t *out, const char *in, int out_size);
char *av_base64_encode(char *out, int out_size, const uint8_t *in, int in_size);

int av_rc4_init(struct AVRC4 *d, const uint8_t *key, int key_bits, int decrypt);
void av_rc4_crypt(struct AVRC4 *d, uint8_t *dst, const uint8_t *src, int count, uint8_t *iv, int decrypt);

int av_lzo1x_decode(void *out, int *outlen, const void *in, int *inlen);

...

yes not all things do, lls.h for example does not, but i think lls should be
changed. That said iam not saying everything should now be changed to
hierachical naming just that the imgutils should.
Thats not just for grep but also for human readability, its nice if when
reading a piece of code one can know if a function is part of the imgutils or
not without having to know all individual functions in there. And no we do
have av_*image functions that are not from imgutils


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

Why not whip the teacher when the pupil misbehaves? -- Diogenes of Sinope
-------------- 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/20100905/e585ae2e/attachment.pgp>