| 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 | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
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
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 --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);
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(-)