diff mbox series

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

Message ID 20200920172443.4763-3-jamrial@gmail.com
State Superseded
Headers show
Series [FFmpeg-devel,1/4] avcodec/cbs: add an AVClass to CodedBitstreamType for option handling
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

James Almer Sept. 20, 2020, 5:24 p.m. UTC
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@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(-)

Comments

Mark Thompson Oct. 2, 2020, 12:23 a.m. UTC | #1
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@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
James Almer Oct. 2, 2020, 12:31 a.m. UTC | #2
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@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@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff mbox series

Patch

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 },
+    { 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,
 
     .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];
+    }
 
     fb(4, frame_width_bits_minus_1);
     fb(4, frame_height_bits_minus_1);