| Message ID | AM7PR03MB66609A0A0E4163230821613D8FF19@AM7PR03MB6660.eurprd03.prod.outlook.com |
|---|---|
| State | Accepted |
| Commit | 417bd4f7dd885b3a5134eb8f86833946bf51afa7 |
| Headers | show |
| Series | [FFmpeg-devel,1/4] avformat/aviobuf: Avoid allocation when using dynamic buffer | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
| andriy/PPC64_make | warning | Make failed |
Andreas Rheinhardt (12021-08-04): > Reordering max_lowres is an ABI break. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > libavcodec/codec.h | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) If it breaks API, then I would rather see it done properly rather than fortuitously: move capabilities and max_lowres after all the pointers. For reference, the robust way of avoiding padding in structures is to order fields by decreasing size. Since the public part of AVCodec changes rarely, once it is done, it should mostly hold. Regards,
On Wed, Aug 4, 2021 at 6:36 PM Nicolas George <george@nsup.org> wrote: > > Andreas Rheinhardt (12021-08-04): > > Reordering max_lowres is an ABI break. > > > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > > --- > > libavcodec/codec.h | 13 +++++++------ > > 1 file changed, 7 insertions(+), 6 deletions(-) > > If it breaks API, then I would rather see it done properly rather than > fortuitously: move capabilities and max_lowres after all the pointers. > > For reference, the robust way of avoiding padding in structures is to > order fields by decreasing size. Since the public part of AVCodec > changes rarely, once it is done, it should mostly hold. > This seems rather drastic for a change with barely any noticeable improvement. I would rather see elements properly grouped by their semantics as makes sense, rather than arbitrarily by size. Related elements appearing together in the header is overall a more important part for readability of the headers, then a few bytes saved. - Hendrik
Hendrik Leppkes (12021-08-04): > This seems rather drastic for a change with barely any noticeable > improvement. I would rather see elements properly grouped by their > semantics as makes sense, rather than arbitrarily by size. > Related elements appearing together in the header is overall a more > important part for readability of the headers, then a few bytes saved. It seems drastic, but it is not really: almost all the fields are pointers, we can group them any way we want. Regards,
Nicolas George: > Andreas Rheinhardt (12021-08-04): >> Reordering max_lowres is an ABI break. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >> --- >> libavcodec/codec.h | 13 +++++++------ >> 1 file changed, 7 insertions(+), 6 deletions(-) > > If it breaks API, then I would rather see it done properly rather than > fortuitously: move capabilities and max_lowres after all the pointers. > > For reference, the robust way of avoiding padding in structures is to > order fields by decreasing size. Since the public part of AVCodec > changes rarely, once it is done, it should mostly hold. > It is about more than just padding; I'd like to see elements that belong together to be together in the structure (like a pointer and a size field, even if the size field is only four bytes); and commonly used elements should be at the front. This makes it easier for API users to get to the important points, it is better for cache locality and it also means that one can sometimes use a smaller instruction in pointer + offset addressing. E.g. on x86 offsets up to 128B need only one byte to encode. Moving caps_internal (the most commonly used of the private fields) to the front of the private part therefore resulted in savings of 48B for me (tiny (actually negligible), I know, but this is already enough to show that ordering everything by decreasing size is not everything). - Andreas
diff --git a/libavcodec/codec.h b/libavcodec/codec.h index cedd106953..8f12705066 100644 --- a/libavcodec/codec.h +++ b/libavcodec/codec.h @@ -214,12 +214,12 @@ typedef struct AVCodec { * see AV_CODEC_CAP_* */ int capabilities; + uint8_t max_lowres; ///< maximum value for lowres supported by the decoder const AVRational *supported_framerates; ///< array of supported framerates, or NULL if any, array is terminated by {0,0} const enum AVPixelFormat *pix_fmts; ///< array of supported pixel formats, or NULL if unknown, array is terminated by -1 const int *supported_samplerates; ///< array of supported audio samplerates, or NULL if unknown, array is terminated by 0 const enum AVSampleFormat *sample_fmts; ///< array of supported sample formats, or NULL if unknown, array is terminated by -1 const uint64_t *channel_layouts; ///< array of support channel layouts, or NULL if unknown. array is terminated by 0 - uint8_t max_lowres; ///< maximum value for lowres supported by the decoder const AVClass *priv_class; ///< AVClass for the private context const AVProfile *profiles; ///< array of recognized profiles, or NULL if unknown, array is terminated by {FF_PROFILE_UNKNOWN} @@ -242,6 +242,12 @@ typedef struct AVCodec { * New public fields should be added right above. ***************************************************************** */ + /** + * Internal codec capabilities. + * See FF_CODEC_CAP_* in internal.h + */ + int caps_internal; + int priv_data_size; /** * @name Frame-level threading support functions @@ -323,11 +329,6 @@ typedef struct AVCodec { * Will be called when seeking */ void (*flush)(struct AVCodecContext *); - /** - * Internal codec capabilities. - * See FF_CODEC_CAP_* in internal.h - */ - int caps_internal; /** * Decoding only, a comma-separated list of bitstream filters to apply to
Reordering max_lowres is an ABI break. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> --- libavcodec/codec.h | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)