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

[James Almer]

On 10/1/2020 9:23 PM, Mark Thompson wrote:
> 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.

It's used in the libdav1d decoder wrapper, so i wanted to keep it
consistent. Admittedly, CBS is not meant to face the user, so i guess i
can change it.

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

Sure.

> 
>> +    { 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.

Ok.

> 
>>         .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.)

Good idea, this is meant to be used only during read after all.

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

I'm copying the current dav1d behavior, where it outputs operating point
0 if you request one that's not available. But i can make it error out,
sure.

> 
>>         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.

You mean for patch 4/4?

> 
> - Mark
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".