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 <jamr...@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-requ...@ffmpeg.org with subject "unsubscribe". _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".