diff mbox series

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

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
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
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(+)

Comments

Mark Thompson Oct. 1, 2020, 11:57 p.m. UTC | #1
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
James Almer Oct. 2, 2020, 12:38 a.m. UTC | #2
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".
James Almer Nov. 15, 2020, 1:16 p.m. UTC | #3
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 mbox series

Patch

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);