[FFmpeg-devel] [PATCH] cbs_h264: Fix types of abs_diff_pic_num_minus1 and

[Andreas Rheinhardt]

Mark Thompson:
> On 07/06/2019 02:17, Andreas Rheinhardt wrote:
>> difference_of_pic_nums_minus1
>>
>> They are unsigned values.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
>> ---
>> Sorry for sending the first email prematurely.
>>  libavcodec/cbs_h264.h | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/libavcodec/cbs_h264.h b/libavcodec/cbs_h264.h
>> index a31be298ba..b9b9d2d1fd 100644
>> --- a/libavcodec/cbs_h264.h
>> +++ b/libavcodec/cbs_h264.h
>> @@ -379,7 +379,7 @@ typedef struct H264RawSliceHeader {
>>      uint8_t ref_pic_list_modification_flag_l1;
>>      struct {
>>          uint8_t modification_of_pic_nums_idc;
>> -        int32_t abs_diff_pic_num_minus1;
>> +        uint32_t abs_diff_pic_num_minus1;
>>          uint8_t long_term_pic_num;
>>      } rplm_l0[H264_MAX_RPLM_COUNT], rplm_l1[H264_MAX_RPLM_COUNT];
>>  
>> @@ -406,7 +406,7 @@ typedef struct H264RawSliceHeader {
>>      uint8_t adaptive_ref_pic_marking_mode_flag;
>>      struct {
>>          uint8_t memory_management_control_operation;
>> -        int32_t difference_of_pic_nums_minus1;
>> +        uint32_t difference_of_pic_nums_minus1;
>>          uint8_t long_term_pic_num;
>>          uint8_t long_term_frame_idx;
>>          uint8_t max_long_term_frame_idx_plus1;
>>
> 
> Not sure of this one - the ranges easily fit in int32_t, and they are likely be used in signed contexts (PicNum for a given reference picture can be negative).  Is there really any benefit to the field being unsigned?

The only benefit is that with this patch applied the type of the
variable matches the parsing process for the variables (they are both
unsigned exponential-golomb variables). And in case of
abs_diff_pic_num_minus1 it also matches the name -- it's an absolute
value, after all.
But your remark that the ranges fit into int32_t is right, of course,
so there is no real bug here.

- Andreas