[FFmpeg-devel] avutil/frame: Remove requirements to use accessors for AVFrame

Submitted by Michael Niedermayer on Jan. 9, 2017, 12:06 p.m.

Details

Message ID 20170109120635.14769-1-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer Jan. 9, 2017, 12:06 p.m.
After this it is not allowed to remove, insert or change fields except to
add them at the end.

The accessors were in the past required to achieve ABI compatibility
with the fork. The community decided long ago that this ABI compatibility
is not needed and should be dropped, so the accessors lost their main
reason for existence.

Several people prefer not using accessors, the fields affected did not
move since 3.0 AFAICS.

Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavutil/frame.h | 19 +------------------
 1 file changed, 1 insertion(+), 18 deletions(-)

Comments

Paul B Mahol Jan. 9, 2017, 12:08 p.m.
On 1/9/17, Michael Niedermayer <michael@niedermayer.cc> wrote:
> After this it is not allowed to remove, insert or change fields except to
> add them at the end.
>
> The accessors were in the past required to achieve ABI compatibility
> with the fork. The community decided long ago that this ABI compatibility
> is not needed and should be dropped, so the accessors lost their main
> reason for existence.
>
> Several people prefer not using accessors, the fields affected did not
> move since 3.0 AFAICS.
>
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavutil/frame.h | 19 +------------------
>  1 file changed, 1 insertion(+), 18 deletions(-)
>

+1
wm4 Jan. 9, 2017, 12:15 p.m.
On Mon,  9 Jan 2017 13:06:35 +0100
Michael Niedermayer <michael@niedermayer.cc> wrote:

> After this it is not allowed to remove, insert or change fields except to
> add them at the end.
> 
> The accessors were in the past required to achieve ABI compatibility
> with the fork. The community decided long ago that this ABI compatibility
> is not needed and should be dropped, so the accessors lost their main
> reason for existence.
> 
> Several people prefer not using accessors, the fields affected did not
> move since 3.0 AFAICS.
> 
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavutil/frame.h | 19 +------------------
>  1 file changed, 1 insertion(+), 18 deletions(-)
> 
> diff --git a/libavutil/frame.h b/libavutil/frame.h
> index b4500923af..38dd767a90 100644
> --- a/libavutil/frame.h
> +++ b/libavutil/frame.h
> @@ -420,8 +420,6 @@ typedef struct AVFrame {
>  
>      /**
>       * MPEG vs JPEG YUV range.
> -     * It must be accessed using av_frame_get_color_range() and
> -     * av_frame_set_color_range().
>       * - encoding: Set by user
>       * - decoding: Set by libavcodec
>       */
> @@ -433,8 +431,6 @@ typedef struct AVFrame {
>  
>      /**
>       * YUV colorspace type.
> -     * It must be accessed using av_frame_get_colorspace() and
> -     * av_frame_set_colorspace().
>       * - encoding: Set by user
>       * - decoding: Set by libavcodec
>       */
> @@ -444,8 +440,6 @@ typedef struct AVFrame {
>  
>      /**
>       * frame timestamp estimated using various heuristics, in stream time base
> -     * Code outside libavutil should access this field using:
> -     * av_frame_get_best_effort_timestamp(frame)
>       * - encoding: unused
>       * - decoding: set by libavcodec, read by user.
>       */
> @@ -453,8 +447,6 @@ typedef struct AVFrame {
>  
>      /**
>       * reordered pos from the last AVPacket that has been input into the decoder
> -     * Code outside libavutil should access this field using:
> -     * av_frame_get_pkt_pos(frame)
>       * - encoding: unused
>       * - decoding: Read by user.
>       */
> @@ -463,8 +455,6 @@ typedef struct AVFrame {
>      /**
>       * duration of the corresponding packet, expressed in
>       * AVStream->time_base units, 0 if unknown.
> -     * Code outside libavutil should access this field using:
> -     * av_frame_get_pkt_duration(frame)
>       * - encoding: unused
>       * - decoding: Read by user.
>       */
> @@ -472,8 +462,6 @@ typedef struct AVFrame {
>  
>      /**
>       * metadata.
> -     * Code outside libavutil should access this field using:
> -     * av_frame_get_metadata(frame)
>       * - encoding: Set by user.
>       * - decoding: Set by libavcodec.
>       */
> @@ -483,8 +471,6 @@ typedef struct AVFrame {
>       * decode error flags of the frame, set to a combination of
>       * FF_DECODE_ERROR_xxx flags if the decoder produced a frame, but there
>       * were errors during the decoding.
> -     * Code outside libavutil should access this field using:
> -     * av_frame_get_decode_error_flags(frame)
>       * - encoding: unused
>       * - decoding: set by libavcodec, read by user.
>       */
> @@ -494,8 +480,6 @@ typedef struct AVFrame {
>  
>      /**
>       * number of audio channels, only used for audio.
> -     * Code outside libavutil should access this field using:
> -     * av_frame_get_channels(frame)
>       * - encoding: unused
>       * - decoding: Read by user.
>       */
> @@ -503,8 +487,7 @@ typedef struct AVFrame {
>  
>      /**
>       * size of the corresponding packet containing the compressed
> -     * frame. It must be accessed using av_frame_get_pkt_size() and
> -     * av_frame_set_pkt_size().
> +     * frame.
>       * It is set to a negative value if unknown.
>       * - encoding: unused
>       * - decoding: set by libavcodec, read by user.

+1

This was broken anyway, because hw_frames_ctx (at the end of AVFrame)
allowed direct access.

Also:

    /**
     * Not to be accessed directly from outside libavutil
     */
    AVBufferRef *qp_table_buf;

The comment should be adjusted and allow direct access. Also,
it should be moved out of the FF_API_FRAME_QP ifdef (why was
it under it anyway).
Ronald S. Bultje Jan. 9, 2017, 1:17 p.m.
Hi,

On Mon, Jan 9, 2017 at 7:15 AM, wm4 <nfxjfg@googlemail.com> wrote:

>     /**
>      * Not to be accessed directly from outside libavutil
>      */
>     AVBufferRef *qp_table_buf;
>
> The comment should be adjusted and allow direct access. Also,
> it should be moved out of the FF_API_FRAME_QP ifdef (why was
> it under it anyway).


I disagree. FF_API_FRAME_QP is not something that exists or is useful
outside a handful of codecs (basically mpeg-1/2/4). If we want this to be
public API, it should exist in a way that makes it useful to the majority
of codecs that use quantization. I'm speaking codecs not developed by
people that implemented qp_table_buf, for example vp3/5/6/8/9 etc.

This also means we need a useful way to normalization the quantizer to a
universal scale. Exporting it as SCALE_MPEG, SCALE_H264 etc. is not useful.
We need a universal Q so -sameq has some kind of meaning when re-encoding
mpeg-4 to vp9. Also, aq visualization in libavfilter would suddenly be
meaningful across codecs. Etc.

Ronald
wm4 Jan. 9, 2017, 1:48 p.m.
On Mon, 9 Jan 2017 08:17:08 -0500
"Ronald S. Bultje" <rsbultje@gmail.com> wrote:

> Hi,
> 
> On Mon, Jan 9, 2017 at 7:15 AM, wm4 <nfxjfg@googlemail.com> wrote:
> 
> >     /**
> >      * Not to be accessed directly from outside libavutil
> >      */
> >     AVBufferRef *qp_table_buf;
> >
> > The comment should be adjusted and allow direct access. Also,
> > it should be moved out of the FF_API_FRAME_QP ifdef (why was
> > it under it anyway).  
> 
> 
> I disagree. FF_API_FRAME_QP is not something that exists or is useful
> outside a handful of codecs (basically mpeg-1/2/4). If we want this to be
> public API, it should exist in a way that makes it useful to the majority
> of codecs that use quantization. I'm speaking codecs not developed by
> people that implemented qp_table_buf, for example vp3/5/6/8/9 etc.
> 
> This also means we need a useful way to normalization the quantizer to a
> universal scale. Exporting it as SCALE_MPEG, SCALE_H264 etc. is not useful.
> We need a universal Q so -sameq has some kind of meaning when re-encoding
> mpeg-4 to vp9. Also, aq visualization in libavfilter would suddenly be
> meaningful across codecs. Etc.

Ah, I thought the QP API survived, but now I see that the accessors are
within FF_API_FRAME_QP as well. So this is not a bug like I thought,
but intentional, and all QP retrieval will be removed after the API
bump. Just strange that the accessors have no attribute_deprecated.
Ronald S. Bultje Jan. 9, 2017, 3:27 p.m.
Hi,

On Mon, Jan 9, 2017 at 8:48 AM, wm4 <nfxjfg@googlemail.com> wrote:

> On Mon, 9 Jan 2017 08:17:08 -0500
> "Ronald S. Bultje" <rsbultje@gmail.com> wrote:
>
> > Hi,
> >
> > On Mon, Jan 9, 2017 at 7:15 AM, wm4 <nfxjfg@googlemail.com> wrote:
> >
> > >     /**
> > >      * Not to be accessed directly from outside libavutil
> > >      */
> > >     AVBufferRef *qp_table_buf;
> > >
> > > The comment should be adjusted and allow direct access. Also,
> > > it should be moved out of the FF_API_FRAME_QP ifdef (why was
> > > it under it anyway).
> >
> >
> > I disagree. FF_API_FRAME_QP is not something that exists or is useful
> > outside a handful of codecs (basically mpeg-1/2/4). If we want this to be
> > public API, it should exist in a way that makes it useful to the majority
> > of codecs that use quantization. I'm speaking codecs not developed by
> > people that implemented qp_table_buf, for example vp3/5/6/8/9 etc.
> >
> > This also means we need a useful way to normalization the quantizer to a
> > universal scale. Exporting it as SCALE_MPEG, SCALE_H264 etc. is not
> useful.
> > We need a universal Q so -sameq has some kind of meaning when re-encoding
> > mpeg-4 to vp9. Also, aq visualization in libavfilter would suddenly be
> > meaningful across codecs. Etc.
>
> Ah, I thought the QP API survived, but now I see that the accessors are
> within FF_API_FRAME_QP as well. So this is not a bug like I thought,
> but intentional, and all QP retrieval will be removed after the API
> bump. Just strange that the accessors have no attribute_deprecated.


I agree that's strange. We should probably add them...

Ronald
Michael Niedermayer Jan. 9, 2017, 5:38 p.m.
On Mon, Jan 09, 2017 at 10:27:09AM -0500, Ronald S. Bultje wrote:
> Hi,
> 
> On Mon, Jan 9, 2017 at 8:48 AM, wm4 <nfxjfg@googlemail.com> wrote:
> 
> > On Mon, 9 Jan 2017 08:17:08 -0500
> > "Ronald S. Bultje" <rsbultje@gmail.com> wrote:
> >
> > > Hi,
> > >
> > > On Mon, Jan 9, 2017 at 7:15 AM, wm4 <nfxjfg@googlemail.com> wrote:
> > >
> > > >     /**
> > > >      * Not to be accessed directly from outside libavutil
> > > >      */
> > > >     AVBufferRef *qp_table_buf;
> > > >
> > > > The comment should be adjusted and allow direct access. Also,
> > > > it should be moved out of the FF_API_FRAME_QP ifdef (why was
> > > > it under it anyway).
> > >
> > >
> > > I disagree. FF_API_FRAME_QP is not something that exists or is useful
> > > outside a handful of codecs (basically mpeg-1/2/4). If we want this to be
> > > public API, it should exist in a way that makes it useful to the majority
> > > of codecs that use quantization. I'm speaking codecs not developed by
> > > people that implemented qp_table_buf, for example vp3/5/6/8/9 etc.
> > >
> > > This also means we need a useful way to normalization the quantizer to a
> > > universal scale. Exporting it as SCALE_MPEG, SCALE_H264 etc. is not
> > useful.
> > > We need a universal Q so -sameq has some kind of meaning when re-encoding
> > > mpeg-4 to vp9. Also, aq visualization in libavfilter would suddenly be
> > > meaningful across codecs. Etc.
> >
> > Ah, I thought the QP API survived, but now I see that the accessors are
> > within FF_API_FRAME_QP as well. So this is not a bug like I thought,
> > but intentional, and all QP retrieval will be removed after the API
> > bump. Just strange that the accessors have no attribute_deprecated.
> 
> 
> I agree that's strange. We should probably add them...

before deprecating the API to access the QP tables a replacement is
needed and uses of teh API should be changed to use the replacement.
Otherwise not only would code using it conflict with a API bump but
also produce warnings about use of deprecated code


[...]
wm4 Jan. 10, 2017, 7:04 a.m.
On Mon, 9 Jan 2017 18:38:04 +0100
Michael Niedermayer <michael@niedermayer.cc> wrote:

> On Mon, Jan 09, 2017 at 10:27:09AM -0500, Ronald S. Bultje wrote:
> > Hi,
> > 
> > On Mon, Jan 9, 2017 at 8:48 AM, wm4 <nfxjfg@googlemail.com> wrote:
> >   
> > > On Mon, 9 Jan 2017 08:17:08 -0500
> > > "Ronald S. Bultje" <rsbultje@gmail.com> wrote:
> > >  
> > > > Hi,
> > > >
> > > > On Mon, Jan 9, 2017 at 7:15 AM, wm4 <nfxjfg@googlemail.com> wrote:
> > > >  
> > > > >     /**
> > > > >      * Not to be accessed directly from outside libavutil
> > > > >      */
> > > > >     AVBufferRef *qp_table_buf;
> > > > >
> > > > > The comment should be adjusted and allow direct access. Also,
> > > > > it should be moved out of the FF_API_FRAME_QP ifdef (why was
> > > > > it under it anyway).  
> > > >
> > > >
> > > > I disagree. FF_API_FRAME_QP is not something that exists or is useful
> > > > outside a handful of codecs (basically mpeg-1/2/4). If we want this to be
> > > > public API, it should exist in a way that makes it useful to the majority
> > > > of codecs that use quantization. I'm speaking codecs not developed by
> > > > people that implemented qp_table_buf, for example vp3/5/6/8/9 etc.
> > > >
> > > > This also means we need a useful way to normalization the quantizer to a
> > > > universal scale. Exporting it as SCALE_MPEG, SCALE_H264 etc. is not  
> > > useful.  
> > > > We need a universal Q so -sameq has some kind of meaning when re-encoding
> > > > mpeg-4 to vp9. Also, aq visualization in libavfilter would suddenly be
> > > > meaningful across codecs. Etc.  
> > >
> > > Ah, I thought the QP API survived, but now I see that the accessors are
> > > within FF_API_FRAME_QP as well. So this is not a bug like I thought,
> > > but intentional, and all QP retrieval will be removed after the API
> > > bump. Just strange that the accessors have no attribute_deprecated.  
> > 
> > 
> > I agree that's strange. We should probably add them...  
> 
> before deprecating the API to access the QP tables a replacement is
> needed and uses of teh API should be changed to use the replacement.
> Otherwise not only would code using it conflict with a API bump but
> also produce warnings about use of deprecated code

FF_API_FRAME_QP seems to remove QP data completely. I got this wrong in
my previous post and thought it affected the old non-AVBuffer data
fields only.

Patch hide | download patch | download mbox

diff --git a/libavutil/frame.h b/libavutil/frame.h
index b4500923af..38dd767a90 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -420,8 +420,6 @@  typedef struct AVFrame {
 
     /**
      * MPEG vs JPEG YUV range.
-     * It must be accessed using av_frame_get_color_range() and
-     * av_frame_set_color_range().
      * - encoding: Set by user
      * - decoding: Set by libavcodec
      */
@@ -433,8 +431,6 @@  typedef struct AVFrame {
 
     /**
      * YUV colorspace type.
-     * It must be accessed using av_frame_get_colorspace() and
-     * av_frame_set_colorspace().
      * - encoding: Set by user
      * - decoding: Set by libavcodec
      */
@@ -444,8 +440,6 @@  typedef struct AVFrame {
 
     /**
      * frame timestamp estimated using various heuristics, in stream time base
-     * Code outside libavutil should access this field using:
-     * av_frame_get_best_effort_timestamp(frame)
      * - encoding: unused
      * - decoding: set by libavcodec, read by user.
      */
@@ -453,8 +447,6 @@  typedef struct AVFrame {
 
     /**
      * reordered pos from the last AVPacket that has been input into the decoder
-     * Code outside libavutil should access this field using:
-     * av_frame_get_pkt_pos(frame)
      * - encoding: unused
      * - decoding: Read by user.
      */
@@ -463,8 +455,6 @@  typedef struct AVFrame {
     /**
      * duration of the corresponding packet, expressed in
      * AVStream->time_base units, 0 if unknown.
-     * Code outside libavutil should access this field using:
-     * av_frame_get_pkt_duration(frame)
      * - encoding: unused
      * - decoding: Read by user.
      */
@@ -472,8 +462,6 @@  typedef struct AVFrame {
 
     /**
      * metadata.
-     * Code outside libavutil should access this field using:
-     * av_frame_get_metadata(frame)
      * - encoding: Set by user.
      * - decoding: Set by libavcodec.
      */
@@ -483,8 +471,6 @@  typedef struct AVFrame {
      * decode error flags of the frame, set to a combination of
      * FF_DECODE_ERROR_xxx flags if the decoder produced a frame, but there
      * were errors during the decoding.
-     * Code outside libavutil should access this field using:
-     * av_frame_get_decode_error_flags(frame)
      * - encoding: unused
      * - decoding: set by libavcodec, read by user.
      */
@@ -494,8 +480,6 @@  typedef struct AVFrame {
 
     /**
      * number of audio channels, only used for audio.
-     * Code outside libavutil should access this field using:
-     * av_frame_get_channels(frame)
      * - encoding: unused
      * - decoding: Read by user.
      */
@@ -503,8 +487,7 @@  typedef struct AVFrame {
 
     /**
      * size of the corresponding packet containing the compressed
-     * frame. It must be accessed using av_frame_get_pkt_size() and
-     * av_frame_set_pkt_size().
+     * frame.
      * It is set to a negative value if unknown.
      * - encoding: unused
      * - decoding: set by libavcodec, read by user.