[FFmpeg-devel] [PATCH 3/4] avcodec/cbs_av1: add an option to select an operating point

[Mark Thompson]

On 20/09/2020 18:24, James Almer wrote:
> This implements the function drop_obu() as defined in Setion 6.2.1 from the
> spec.
> In a reading only scenario, units that belong to an operating point the
> caller doesn't want should not be parsed.
> 
> Signed-off-by: James Almer <jamrial at gmail.com>
> ---
>   libavcodec/cbs_av1.c                 | 18 +++++++++++++++++-
>   libavcodec/cbs_av1.h                 |  5 +++++
>   libavcodec/cbs_av1_syntax_template.c |  7 +++++++
>   3 files changed, 29 insertions(+), 1 deletion(-)

I think the AVClass and option of patch 1 and this seems like a sensible approach.

> diff --git a/libavcodec/cbs_av1.c b/libavcodec/cbs_av1.c
> index dcf6c140ae..edacc29ca8 100644
> --- a/libavcodec/cbs_av1.c
> +++ b/libavcodec/cbs_av1.c
> @@ -18,6 +18,7 @@
>   
>   #include "libavutil/avassert.h"
>   #include "libavutil/pixfmt.h"
> +#include "libavutil/opt.h"
>   
>   #include "cbs.h"
>   #include "cbs_internal.h"
> @@ -883,7 +884,7 @@ static int cbs_av1_read_unit(CodedBitstreamContext *ctx,
>               int in_spatial_layer  =
>                   (priv->operating_point_idc >> (priv->spatial_id + 8)) & 1;
>               if (!in_temporal_layer || !in_spatial_layer) {
> -                // Decoding will drop this OBU at this operating point.
> +                return AVERROR(EAGAIN); // drop_obu()
>               }
>           }
>       }
> @@ -1238,10 +1239,25 @@ static const CodedBitstreamUnitTypeDescriptor cbs_av1_unit_types[] = {
>       CBS_UNIT_TYPE_END_OF_LIST
>   };
>   
> +#define OFFSET(x) offsetof(CodedBitstreamAV1Context, x)
> +static const AVOption cbs_av1_options[] = {
> +    { "oppoint",  "Select an operating point of the scalable bitstream", OFFSET(operating_point),
> +                  AV_OPT_TYPE_INT, { .i64 = -1 }, -1, AV1_MAX_OPERATING_POINTS - 1, 0 },

"oppoint" isn't used as an abbreviation anywhere in the standard, only "op" (so, operating point point?).  There isn't really any reason to shorten it, so just having "operating_point" would seem clearer.

For the help string, maybe something a little nicer like "Set operating point to select layers to decode from a scalable bitstream"?

> +    { NULL }
> +};
> +
> +static const AVClass cbs_av1_class = {
> +    .class_name = "cbs_av1",
> +    .item_name  = av_default_item_name,
> +    .option     = cbs_av1_options,
> +    .version    = LIBAVUTIL_VERSION_INT,
> +};
> +
>   const CodedBitstreamType ff_cbs_type_av1 = {
>       .codec_id          = AV_CODEC_ID_AV1,
>   
>       .priv_data_size    = sizeof(CodedBitstreamAV1Context),
> +    .priv_class        = &cbs_av1_class,

Not in the same order as patch 1.  The way around doesn't matter, but keep it consistent.

>   
>       .unit_types        = cbs_av1_unit_types,
>   
> diff --git a/libavcodec/cbs_av1.h b/libavcodec/cbs_av1.h
> index 7a0c08c596..27b44d68ff 100644
> --- a/libavcodec/cbs_av1.h
> +++ b/libavcodec/cbs_av1.h
> @@ -416,6 +416,8 @@ typedef struct AV1ReferenceFrameState {
>   } AV1ReferenceFrameState;
>   
>   typedef struct CodedBitstreamAV1Context {
> +    const AVClass *class;
> +
>       AV1RawSequenceHeader *sequence_header;
>       AVBufferRef          *sequence_header_ref;
>   
> @@ -443,6 +445,9 @@ typedef struct CodedBitstreamAV1Context {
>       int tile_rows;
>   
>       AV1ReferenceFrameState ref[AV1_NUM_REF_FRAMES];
> +
> +    // AVOptions
> +    int operating_point;
>   } CodedBitstreamAV1Context;
>   
>   
> diff --git a/libavcodec/cbs_av1_syntax_template.c b/libavcodec/cbs_av1_syntax_template.c
> index bcaa334134..34d09fab68 100644
> --- a/libavcodec/cbs_av1_syntax_template.c
> +++ b/libavcodec/cbs_av1_syntax_template.c
> @@ -186,6 +186,7 @@ static int FUNC(decoder_model_info)(CodedBitstreamContext *ctx, RWContext *rw,
>   static int FUNC(sequence_header_obu)(CodedBitstreamContext *ctx, RWContext *rw,
>                                        AV1RawSequenceHeader *current)
>   {
> +    CodedBitstreamAV1Context *priv = ctx->priv_data;
>       int i, err;
>   
>       HEADER("Sequence Header");
> @@ -253,6 +254,12 @@ static int FUNC(sequence_header_obu)(CodedBitstreamContext *ctx, RWContext *rw,
>               }
>           }
>       }
> +    if (priv->operating_point >= 0) {
> +        int op_pt = 0;
> +        if (priv->operating_point <= current->operating_points_cnt_minus_1)
> +            op_pt = priv->operating_point;
> +        priv->operating_point_idc = current->operating_point_idc[op_pt];
> +    }

Would it maybe make more sense to put this near cbs_av1.c:900 to only apply to read rather than having it in the parsing template?  (I know there is choose_operating_point() there in the standard, but it doesn't affect any of the sequence header.)

This probably wants an error message if the selected operating point isn't valid, rather than silently ignoring the option.

>   
>       fb(4, frame_width_bits_minus_1);
>       fb(4, frame_height_bits_minus_1);
> 

Might be a good idea to document this in doc/decoders.texi.

- Mark