| Message ID | 20170109120635.14769-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
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
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).
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
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.
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
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 [...]
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.
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.
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(-)