[FFmpeg-devel] [PATCH] avcodec/h264_ps: fix storage size for offset_for_ref_frame

James Almer jamrial at gmail.com
Wed Apr 17 01:01:50 EEST 2019


On 4/16/2019 6:48 PM, Mark Thompson wrote:
> On 11/04/2019 04:10, James Almer wrote:
>> On 4/10/2019 3:30 PM, James Almer wrote:
>>> The spec defines the valid range of values to be INT32_MIN + 1 to INT32_MAX, inclusive.
>>>
>>> Signed-off-by: James Almer <jamrial at gmail.com>
>>> ---
>>> A good example of why making offsets and sizes of structs like this tied to the
>>> ABI is not a good idea.
>>>
>>>  libavcodec/h264_ps.h | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/libavcodec/h264_ps.h b/libavcodec/h264_ps.h
>>> index e967b9cbcf..9014326dfb 100644
>>> --- a/libavcodec/h264_ps.h
>>> +++ b/libavcodec/h264_ps.h
>>> @@ -81,7 +81,7 @@ typedef struct SPS {
>>>      uint32_t num_units_in_tick;
>>>      uint32_t time_scale;
>>>      int fixed_frame_rate_flag;
>>> -    short offset_for_ref_frame[256]; // FIXME dyn aloc?
>>> +    int32_t offset_for_ref_frame[256];
>>
>> The doxy for get_se_golomb() doesn't mention the range of values it can
>> handle, but seeing there's also a get_se_golomb_long(), I guess the
>> relevant line in h264_ps.c should now use the latter instead?
> 
> I think it's correct to do that.  Seems highly unlikely anyone would ever hit it outside a conformance-checking context, though - using anything other than pic_order_cnt_type 0 for nontrivial reference structures is madness.
> 
>>>      int bitstream_restriction_flag;
>>>      int num_reorder_frames;
>>>      int scaling_matrix_present;
> 
> There are some other fields with int32_t range which are using get_se_golomb() - e.g. offset_for_non_ref_pic.  I guess they should use get_se_golomb_long() as above.  They're also plain ints - do they want to be explicitly int32_t?

It's wildly inconsistent. There are both scalar values and arrays as
uint32_t, int, and unsigned it, but in all cases they can store the
correct range of values, so IMO, not worth changing unless the whole
structs are made consistent, like you did with the CBS ones.

Which are these? I'll send a separate patch changing get_se_golomb() to
get_se_golomb_long() for all of them.

> 
> - Mark
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
> 



More information about the ffmpeg-devel mailing list