[FFmpeg-devel] [PATCH 2/2] avcodec/cbs_h266: store RowHeightVal in the context

[James Almer]

On 7/1/2023 7:33 PM, James Almer wrote:
> On 7/1/2023 12:10 PM, Nuo Mi wrote:
>> On Sat, Jul 1, 2023 at 9:37 AM James Almer <jamrial at gmail.com> wrote:
>>
>>> Stop overwriting values from the bitstream array
>>> pps_tile_row_height_minus1.
>>>
>>> Signed-off-by: James Almer <jamrial at gmail.com>
>>> ---
>>>   libavcodec/cbs_h266_syntax_template.c | 27 ++++++++++++++-------------
>>>   1 file changed, 14 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/libavcodec/cbs_h266_syntax_template.c
>>> b/libavcodec/cbs_h266_syntax_template.c
>>> index ec2bb1ccc3..625995a2bd 100644
>>> --- a/libavcodec/cbs_h266_syntax_template.c
>>> +++ b/libavcodec/cbs_h266_syntax_template.c
>>> @@ -1779,14 +1779,14 @@ static int FUNC(pps) (CodedBitstreamContext 
>>> *ctx,
>>> RWContext *rw,
>>>                        "Tile row height(%d) exceeds picture 
>>> height\n",i);
>>>                 return AVERROR_INVALIDDATA;
>>>             }
>>> +          current->row_height_val[i] =
>>> current->pps_tile_row_height_minus1[i] + 1;
>>>             remaining_size -= (current->pps_tile_row_height_minus1[i] 
>>> + 1);
>>>           }
>>> -        unified_size = (i == 0 ? pic_height_in_ctbs_y :
>>> -                        (current->pps_tile_row_height_minus1[i - 1] 
>>> + 1));
>>> +        unified_size = current->pps_tile_row_height_minus1[i - 1] + 1;
>>>
>>>           while (remaining_size > 0) {
>>>               unified_size = FFMIN(remaining_size, unified_size);
>>> -            current->pps_tile_row_height_minus1[i] = unified_size - 1;
>>> +            current->row_height_val[i] = unified_size;
>>>               remaining_size -= unified_size;
>>>               i++;
>>>           }
>>> @@ -1855,17 +1855,17 @@ static int FUNC(pps) (CodedBitstreamContext 
>>> *ctx,
>>> RWContext *rw,
>>>                       ctu_x += 
>>> current->pps_tile_column_width_minus1[j] + 1;
>>>                   }
>>>                   for (j = 0; j < tile_y; j++) {
>>> -                    ctu_y += current->pps_tile_row_height_minus1[j] 
>>> + 1;
>>> +                    ctu_y += current->row_height_val[j];
>>>                   }
>>>                   if (current->pps_slice_width_in_tiles_minus1[i] == 
>>> 0 &&
>>>                       current->pps_slice_height_in_tiles_minus1[i] == 
>>> 0 &&
>>> -                    current->pps_tile_row_height_minus1[tile_y] > 0) {
>>> +                    current->row_height_val[tile_y] > 1) {
>>>                       int num_slices_in_tile,
>>>                           uniform_slice_height, 
>>> remaining_height_in_ctbs_y;
>>>                       remaining_height_in_ctbs_y =
>>> -                        current->pps_tile_row_height_minus1[tile_y] 
>>> + 1;
>>> +                        current->row_height_val[tile_y];
>>>                       ues(pps_num_exp_slices_in_tile[i],
>>> -                        0, current->pps_tile_row_height_minus1[tile_y],
>>> 1, i);
>>> +                        0, current->row_height_val[tile_y] - 1, 1, i);
>>>                       if (current->pps_num_exp_slices_in_tile[i] == 0) {
>>>                           num_slices_in_tile = 1;
>>>                           slice_top_left_ctu_x[i] = ctu_x;
>>> @@ -1875,7 +1875,7 @@ static int FUNC(pps) (CodedBitstreamContext *ctx,
>>> RWContext *rw,
>>>                           for (j = 0; j <
>>> current->pps_num_exp_slices_in_tile[i];
>>>                                j++) {
>>>
>>>   ues(pps_exp_slice_height_in_ctus_minus1[i][j], 0,
>>> -
>>> current->pps_tile_row_height_minus1[tile_y], 2,
>>> +                                current->row_height_val[tile_y] - 1, 2,
>>>
>> The benefit is not so obvious when we need to -1 in multiple places.
> 
> It's not about having a benefit, but to stop writing derived values to a 
> raw bitstream struct field. And in the end, i also remove a bunch of +1.
> 
> What i did not carefully look in the spec is which of these uses 
> actually needs pps_tile_row_height_minus1 and which RowHeightVal. I 
> assumed all wanted the latter since it's calculated almost immediately 
> after the former is read from the bitstream, but maybe you know better.

Looking at the spec, it's indeed RowHeightVal and ColWidthVal what's 
used everywhere, including the couple - 1 where i added them.

> 
> I'll send an updated patch to do the same for ColWidthVal, now that i 
> notice the same happens with it.