[FFmpeg-devel] [PATCH 1/3] lavc: implement accessors for some AVFrame fields.

[Clément Bœsch]

On Sun, Apr 29, 2012 at 11:21:44AM +0200, Nicolas George wrote:
> Compared to av_opt_ptr, accessors bring:
> 
> - better performance (negligible);
> - compile-time type check;
> - link-time existence check
>   (or at worst, a dynamic linker error instead of a NULL dereference).
> 

I like the idea, but by the way, should we make this for all the
user-editable/readable fields, with a doxy associated with them?

"sometimes there is a getter/setter, sometimes not; how am I supposed to
know if I can change this given field?"

It might be a lot of efforts to do so though.

> Signed-off-by: Nicolas George <nicolas.george at normalesup.org>
> ---
>  libavcodec/avcodec.h |   22 ++++++++++++++++++----
>  libavcodec/utils.c   |    9 +++++++++
>  libavcodec/version.h |    2 +-
>  3 files changed, 28 insertions(+), 5 deletions(-)
> 
> 
> Considering code size: (on x86_64)
>  - each accessor is 8 bytes of machine code plus 8 bytes of padding;
>  - the call site goes from 21 bytes (using av_opt_str) to 8 bytes.
> 
> Another benefit is that we can decide to make one of the accessors
> non-trivial if needs be.
> 
> 
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index ba51ec1..b1f4fa7 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -1243,7 +1243,7 @@ typedef struct AVFrame {
>      /**
>       * frame timestamp estimated using various heuristics, in stream time base
>       * Code outside libavcodec should access this field using:
> -     *  av_opt_ptr(avcodec_get_frame_class(), frame, "best_effort_timestamp");
> +     * av_frame_get_best_effort_timestamp(frame)
>       * - encoding: unused
>       * - decoding: set by libavcodec, read by user.
>       */
> @@ -1252,7 +1252,7 @@ typedef struct AVFrame {
>      /**
>       * reordered pos from the last AVPacket that has been input into the decoder
>       * Code outside libavcodec should access this field using:
> -     *  av_opt_ptr(avcodec_get_frame_class(), frame, "pkt_pos");
> +     * av_frame_get_pkt_pos(frame)
>       * - encoding: unused
>       * - decoding: Read by user.
>       */
> @@ -1263,7 +1263,7 @@ typedef struct AVFrame {
>       * - encoding: unused
>       * - decoding: read by user.
>       * Code outside libavcodec should access this field using:
> -     * av_opt_ptr(avcodec_get_frame_class(), frame, "channel_layout")
> +     * av_frame_get_channel_layout(frame)
>       */
>      int64_t channel_layout;
>  
> @@ -1272,12 +1272,26 @@ typedef struct AVFrame {
>       * - encoding: unused
>       * - decoding: read by user.
>       * Code outside libavcodec should access this field using:
> -     * av_opt_ptr(avcodec_get_frame_class(), frame, "sample_rate")
> +     * av_frame_get_channel_layout(frame)
>       */
>      int sample_rate;
>  
>  } AVFrame;
>  
> +/**
> + * Accessors for some AVFrame fields.
> + * The position of these field in the structure is not part of the ABI,
> + * they should not be accessed directly outside libavcodec.
> + */
> +int64_t av_frame_get_best_effort_timestamp(const AVFrame *frame);
> +int64_t av_frame_get_pkt_pos              (const AVFrame *frame);
> +int64_t av_frame_get_channel_layout       (const AVFrame *frame);
> +int     av_frame_get_sample_rate          (const AVFrame *frame);
> +void    av_frame_set_best_effort_timestamp(AVFrame *frame, int64_t val);
> +void    av_frame_set_pkt_pos              (AVFrame *frame, int64_t val);
> +void    av_frame_set_channel_layout       (AVFrame *frame, int64_t val);
> +void    av_frame_set_sample_rate          (AVFrame *frame, int     val);
> +
>  struct AVCodecInternal;
>  
>  enum AVFieldOrder {
> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> index 4b6ddea..71227e9 100644
> --- a/libavcodec/utils.c
> +++ b/libavcodec/utils.c
> @@ -672,6 +672,15 @@ AVFrame *avcodec_alloc_frame(void){
>      return pic;
>  }
>  
> +#define MAKE_ACCESSORS(str, name, type, field) \
> +    type av_##name##_get_##field(const str *s) { return s->field; } \
> +    void av_##name##_set_##field(str *s, type v) { s->field = v; }
> +
> +MAKE_ACCESSORS(AVFrame, frame, int64_t, best_effort_timestamp)
> +MAKE_ACCESSORS(AVFrame, frame, int64_t, pkt_pos)
> +MAKE_ACCESSORS(AVFrame, frame, int64_t, channel_layout)
> +MAKE_ACCESSORS(AVFrame, frame, int,     sample_rate)
> +

These fields looks unused at encoding, should we make some setter for them
anyway?

[...]

-- 
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120429/75c1e974/attachment.asc>