[FFmpeg-devel] [PATCH 2/4] avcodec/cbs: allow cbs_read_fragment_content() to discard units

[James Almer]

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