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

Submitted by James Almer on April 10, 2019, 6:30 p.m.

Details

Message ID 20190410183003.2056-1-jamrial@gmail.com
State Accepted
Commit 53cc3338f7b24cda40ad8a69e12b37e7335f37ff
Headers show

Commit Message

James Almer April 10, 2019, 6:30 p.m.
The spec defines the valid range of values to be INT32_MIN + 1 to INT32_MAX, inclusive.

Signed-off-by: James Almer <jamrial@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(-)

Comments

James Almer April 11, 2019, 3:10 a.m.
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@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?

>      int bitstream_restriction_flag;
>      int num_reorder_frames;
>      int scaling_matrix_present;
>
James Almer April 16, 2019, 6:10 p.m.
On 4/11/2019 12:10 AM, 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@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?
> 

Ping.

Also, could use another pair of eyes to make sure other fields have an
storage type that can handle their defined range of valid values.
Mark Thompson April 16, 2019, 9:48 p.m.
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@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?

- Mark
James Almer April 16, 2019, 10:01 p.m.
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@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@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
Baptiste Coudurier April 16, 2019, 10:03 p.m.
> On Apr 16, 2019, at 3:01 PM, James Almer <jamrial@gmail.com> wrote:
> 
> 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@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.

Sounds like a good opportunity to make the whole structs consistent.

—
Baptiste
James Almer April 24, 2019, 9:37 p.m.
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@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?

Pushed this patch, and another making offset_for_ref_frame,
offset_for_non_ref_pic, and offset_for_top_to_bottom_field use
get_se_golomb_long().

Thanks.

Patch hide | download patch | download mbox

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];
     int bitstream_restriction_flag;
     int num_reorder_frames;
     int scaling_matrix_present;