Re: [FFmpeg-devel] [PATCH] av1dec: handle dimension changes via get_format

[James Almer <jamrial@gmail.com>]

On 6/21/2023 6:42 PM, Dave Airlie wrote:
> On Thu, 22 Jun 2023 at 07:36, James Almer <jamrial@gmail.com> wrote:
>>
>> On 6/21/2023 6:15 PM, Dave Airlie wrote:
>>> On Thu, 22 Jun 2023 at 02:36, James Almer <jamrial@gmail.com> wrote:
>>>>
>>>> On 6/20/2023 8:36 PM, airlied@gmail.com wrote:
>>>>> From: Dave Airlie <airlied@redhat.com>
>>>>>
>>>>> av1-1-b8-03-sizeup.ivf on vulkan causes gpu hangs as none of the
>>>>> images get resized when dimensions change, this detects the dim
>>>>> change and calls the get_format to reinit the context.
>>>>> ---
>>>>>     libavcodec/av1dec.c | 12 ++++++++----
>>>>>     1 file changed, 8 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c
>>>>> index e7f98a6c81..1cec328563 100644
>>>>> --- a/libavcodec/av1dec.c
>>>>> +++ b/libavcodec/av1dec.c
>>>>> @@ -721,6 +721,7 @@ static av_cold int av1_decode_free(AVCodecContext *avctx)
>>>>>     }
>>>>>
>>>>>     static int set_context_with_sequence(AVCodecContext *avctx,
>>>>> +                                     int *dim_change,
>>>>>                                          const AV1RawSequenceHeader *seq)
>>>>>     {
>>>>>         int width = seq->max_frame_width_minus_1 + 1;
>>>>> @@ -753,6 +754,8 @@ static int set_context_with_sequence(AVCodecContext *avctx,
>>>>>             int ret = ff_set_dimensions(avctx, width, height);
>>>>>             if (ret < 0)
>>>>>                 return ret;
>>>>> +        if (dim_change)
>>>>> +            *dim_change = 1;
>>>>>         }
>>>>>         avctx->sample_aspect_ratio = (AVRational) { 1, 1 };
>>>>>
>>>>> @@ -859,7 +862,7 @@ static av_cold int av1_decode_init(AVCodecContext *avctx)
>>>>>                 goto end;
>>>>>             }
>>>>>
>>>>> -        ret = set_context_with_sequence(avctx, seq);
>>>>> +        ret = set_context_with_sequence(avctx, NULL, seq);
>>>>>             if (ret < 0) {
>>>>>                 av_log(avctx, AV_LOG_WARNING, "Failed to set decoder context.\n");
>>>>>                 goto end;
>>>>> @@ -1202,7 +1205,7 @@ static int av1_receive_frame_internal(AVCodecContext *avctx, AVFrame *frame)
>>>>>             CodedBitstreamUnit *unit = &s->current_obu.units[i];
>>>>>             AV1RawOBU *obu = unit->content;
>>>>>             const AV1RawOBUHeader *header;
>>>>> -
>>>>> +        int dim_change = 0;
>>>>>             if (!obu)
>>>>>                 continue;
>>>>>
>>>>> @@ -1220,7 +1223,8 @@ static int av1_receive_frame_internal(AVCodecContext *avctx, AVFrame *frame)
>>>>>
>>>>>                 s->raw_seq = &obu->obu.sequence_header;
>>>>>
>>>>> -            ret = set_context_with_sequence(avctx, s->raw_seq);
>>>>> +            dim_change = 0;
>>>>> +            ret = set_context_with_sequence(avctx, &dim_change, s->raw_seq);
>>>>>                 if (ret < 0) {
>>>>>                     av_log(avctx, AV_LOG_ERROR, "Failed to set context.\n");
>>>>>                     s->raw_seq = NULL;
>>>>> @@ -1229,7 +1233,7 @@ static int av1_receive_frame_internal(AVCodecContext *avctx, AVFrame *frame)
>>>>>
>>>>>                 s->operating_point_idc = s->raw_seq->operating_point_idc[s->operating_point];
>>>>>
>>>>> -            if (s->pix_fmt == AV_PIX_FMT_NONE) {
>>>>> +            if (s->pix_fmt == AV_PIX_FMT_NONE || dim_change) {
>>>>>                     ret = get_pixel_format(avctx);
>>>>
>>>> Dimensions can change between frames without a seq header showing up to
>>>> change the max_width/height values. get_pixel_format() would need to be
>>>> called on frame headers instead.
>>>
>>> It can but my reading of the spec is that it is illegal to go beyond
>>> the max in sequence header.
>>>
>>> 6.8 Frame header OBU semantics
>>> 6.8.4 Frame size semantics
>>>
>>> "
>>> It is a requirement of bitstream conformance that frame_width_minus_1
>>> is less than or equal to max_frame_width_minus_1.
>>> It is a requirement of bitstream conformance that frame_height_minus_1
>>> is less than or equal to max_frame_height_minus_1.
>>> "
>>>
>>> Dave.
>>
>> So Vulkan always allocates buffers for the max allowed dimensions and
>> not for the current frame's dimensions?
> 
> It doesn't seem to be in vulkan specific code:
> 
> av1dec.c:set_context_with_sequence
>      int width = seq->max_frame_width_minus_1 + 1;
>      int height = seq->max_frame_height_minus_1 + 1;
> 
> is where it sets the values later used to allocate the frames.
> 
> Dave.

I see a call to ff_set_dimensions() in 
update_context_with_frame_header() which uses frame header dimensions.
_______________________________________________
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".