[FFmpeg-devel] [PATCH 3/5] avcodec/cbs_h264: fix storage type for time_offset in Pic Timing SEI

[James Almer]

On 4/16/2019 8:00 PM, Mark Thompson wrote:
> On 15/04/2019 22:17, James Almer wrote:
>> The spec defines it as a signed value.
>>
>> Signed-off-by: James Almer <jamrial at gmail.com>
>> ---
>> The only sample i could find with time_offset values it's in the fate suite,
>> and in all cases it's 0.
>>
>>  libavcodec/cbs_h264.h                 | 2 +-
>>  libavcodec/cbs_h264_syntax_template.c | 5 +++--
>>  2 files changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/libavcodec/cbs_h264.h b/libavcodec/cbs_h264.h
>> index 92277e4750..b5eee7c370 100644
>> --- a/libavcodec/cbs_h264.h
>> +++ b/libavcodec/cbs_h264.h
>> @@ -253,7 +253,7 @@ typedef struct H264RawSEIPicTimestamp {
>>      uint8_t minutes_value;
>>      uint8_t hours_flag;
>>      uint8_t hours_value;
>> -    uint32_t time_offset;
>> +    int32_t time_offset;
>>  } H264RawSEIPicTimestamp;
>>  
>>  typedef struct H264RawSEIPicTiming {
>> diff --git a/libavcodec/cbs_h264_syntax_template.c b/libavcodec/cbs_h264_syntax_template.c
>> index 4da4c5da67..07b4cddb5e 100644
>> --- a/libavcodec/cbs_h264_syntax_template.c
>> +++ b/libavcodec/cbs_h264_syntax_template.c
>> @@ -592,8 +592,9 @@ static int FUNC(sei_pic_timestamp)(CodedBitstreamContext *ctx, RWContext *rw,
>>          time_offset_length = 24;
>>  
>>      if (time_offset_length > 0)
>> -        u(time_offset_length, time_offset,
>> -          0, MAX_UINT_BITS(time_offset_length));
>> +        i(time_offset_length, time_offset,
>> +          MIN_INT_BITS(time_offset_length),
>> +          MAX_INT_BITS(time_offset_length));
>>      else
>>          infer(time_offset, 0);
>>  
>>
> 
> LGTM.
> 
> I'm glad the standard gets plenty of use out of that i(v) syntax element type definition.
> 
> - Mark
Heh, true, on h264 this is indeed the only case where it's used. On h265
it's also used in the spherical metadata SEI, added in a recent revision
of the spec, so it's not surprising that most implementations got it wrong.

Couldn't find any sample with the spherical SEI, for that matter. All
samples used instead the relevant isobmff boxes.