| Message ID | 20200920172443.4763-2-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: > The caller may not need all units in a fragment in reading only scenarios. They > could in fact alter global state stored in the private CodedBitstreamType > fields in an undesirable way. > And unlike preventing decomposition of units, discarding can be done based on > parsed values within the unit. > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavcodec/cbs.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c > index e73e18f398..363385b6f3 100644 > --- a/libavcodec/cbs.c > +++ b/libavcodec/cbs.c > @@ -196,6 +196,11 @@ static int cbs_read_fragment_content(CodedBitstreamContext *ctx, > av_log(ctx->log_ctx, AV_LOG_VERBOSE, > "Decomposition unimplemented for unit %d " > "(type %"PRIu32").\n", i, unit->type); > + } else if (err == AVERROR(EAGAIN)) { > + av_log(ctx->log_ctx, AV_LOG_VERBOSE, > + "Discarding unit %d " > + "(type %"PRIu32").\n", i, unit->type); > + ff_cbs_delete_unit(frag, i--); > } else if (err < 0) { > av_log(ctx->log_ctx, AV_LOG_ERROR, "Failed to read unit %d " > "(type %"PRIu32").\n", i, unit->type); > I don't really like how it is modifying the units in the fragment internally in the read function. EAGAIN as an "I didn't do anything with this" return from read_unit seems reasonable, but then deleting the unit here feels outside the intended meaning of the function. Would it make sense to push that further out somehow? E.g. av1dec.c ignoring undecomposed units, or maybe a separate function to do deletion under whatever conditions. - Mark
On 10/1/2020 8:57 PM, Mark Thompson wrote: > On 20/09/2020 18:24, James Almer wrote: >> The caller may not need all units in a fragment in reading only >> scenarios. They >> could in fact alter global state stored in the private CodedBitstreamType >> fields in an undesirable way. >> And unlike preventing decomposition of units, discarding can be done >> based on >> parsed values within the unit. >> >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> libavcodec/cbs.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c >> index e73e18f398..363385b6f3 100644 >> --- a/libavcodec/cbs.c >> +++ b/libavcodec/cbs.c >> @@ -196,6 +196,11 @@ static int >> cbs_read_fragment_content(CodedBitstreamContext *ctx, >> av_log(ctx->log_ctx, AV_LOG_VERBOSE, >> "Decomposition unimplemented for unit %d " >> "(type %"PRIu32").\n", i, unit->type); >> + } else if (err == AVERROR(EAGAIN)) { >> + av_log(ctx->log_ctx, AV_LOG_VERBOSE, >> + "Discarding unit %d " >> + "(type %"PRIu32").\n", i, unit->type); >> + ff_cbs_delete_unit(frag, i--); >> } else if (err < 0) { >> av_log(ctx->log_ctx, AV_LOG_ERROR, "Failed to read unit >> %d " >> "(type %"PRIu32").\n", i, unit->type); >> > > I don't really like how it is modifying the units in the fragment > internally in the read function. EAGAIN as an "I didn't do anything > with this" return from read_unit seems reasonable, but then deleting the > unit here feels outside the intended meaning of the function. > > Would it make sense to push that further out somehow? E.g. av1dec.c > ignoring undecomposed units, or maybe a separate function to do deletion > under whatever conditions. Seems overtly complicated for what ultimately will be the same result. The fragment will have only the units that were not discarded before being returned to the caller. And the caller (av1dec) can't really know if a unit was decomposed or not, assuming they are not removed from the fragment. In the case of Patch 3/4, it will return EAGAIN after having filled the OBU header bits. > > - 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".
On 10/1/2020 9:38 PM, James Almer wrote: > On 10/1/2020 8:57 PM, Mark Thompson wrote: >> On 20/09/2020 18:24, James Almer wrote: >>> The caller may not need all units in a fragment in reading only >>> scenarios. They >>> could in fact alter global state stored in the private CodedBitstreamType >>> fields in an undesirable way. >>> And unlike preventing decomposition of units, discarding can be done >>> based on >>> parsed values within the unit. >>> >>> Signed-off-by: James Almer <jamrial@gmail.com> >>> --- >>> libavcodec/cbs.c | 5 +++++ >>> 1 file changed, 5 insertions(+) >>> >>> diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c >>> index e73e18f398..363385b6f3 100644 >>> --- a/libavcodec/cbs.c >>> +++ b/libavcodec/cbs.c >>> @@ -196,6 +196,11 @@ static int >>> cbs_read_fragment_content(CodedBitstreamContext *ctx, >>> av_log(ctx->log_ctx, AV_LOG_VERBOSE, >>> "Decomposition unimplemented for unit %d " >>> "(type %"PRIu32").\n", i, unit->type); >>> + } else if (err == AVERROR(EAGAIN)) { >>> + av_log(ctx->log_ctx, AV_LOG_VERBOSE, >>> + "Discarding unit %d " >>> + "(type %"PRIu32").\n", i, unit->type); >>> + ff_cbs_delete_unit(frag, i--); >>> } else if (err < 0) { >>> av_log(ctx->log_ctx, AV_LOG_ERROR, "Failed to read unit >>> %d " >>> "(type %"PRIu32").\n", i, unit->type); >>> >> >> I don't really like how it is modifying the units in the fragment >> internally in the read function. EAGAIN as an "I didn't do anything >> with this" return from read_unit seems reasonable, but then deleting the >> unit here feels outside the intended meaning of the function. >> >> Would it make sense to push that further out somehow? E.g. av1dec.c >> ignoring undecomposed units, or maybe a separate function to do deletion >> under whatever conditions. > > Seems overtly complicated for what ultimately will be the same result. > The fragment will have only the units that were not discarded before > being returned to the caller. > > And the caller (av1dec) can't really know if a unit was decomposed or > not, assuming they are not removed from the fragment. In the case of > Patch 3/4, it will return EAGAIN after having filled the OBU header bits. Ping? I'd like to apply patches 1 and 2 from this set (or some variation) and v2 of patches 3 and 4.
diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c index e73e18f398..363385b6f3 100644 --- a/libavcodec/cbs.c +++ b/libavcodec/cbs.c @@ -196,6 +196,11 @@ static int cbs_read_fragment_content(CodedBitstreamContext *ctx, av_log(ctx->log_ctx, AV_LOG_VERBOSE, "Decomposition unimplemented for unit %d " "(type %"PRIu32").\n", i, unit->type); + } else if (err == AVERROR(EAGAIN)) { + av_log(ctx->log_ctx, AV_LOG_VERBOSE, + "Discarding unit %d " + "(type %"PRIu32").\n", i, unit->type); + ff_cbs_delete_unit(frag, i--); } else if (err < 0) { av_log(ctx->log_ctx, AV_LOG_ERROR, "Failed to read unit %d " "(type %"PRIu32").\n", i, unit->type);
The caller may not need all units in a fragment in reading only scenarios. They could in fact alter global state stored in the private CodedBitstreamType fields in an undesirable way. And unlike preventing decomposition of units, discarding can be done based on parsed values within the unit. Signed-off-by: James Almer <jamrial@gmail.com> --- libavcodec/cbs.c | 5 +++++ 1 file changed, 5 insertions(+)