| Message ID | 20190410183003.2056-1-jamrial@gmail.com |
|---|---|
| State | Accepted |
| Commit | 53cc3338f7b24cda40ad8a69e12b37e7335f37ff |
| Headers | show |
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; >
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.
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
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". >
> 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
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.
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;
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(-)