[FFmpeg-devel] [PATCH 13/18] h264_sei: parse the picture timing SEIs correctly

[James Almer]

On 3/16/2020 11:59 AM, Anton Khirnov wrote:
> Quoting James Almer (2020-03-13 16:56:12)
>> On 3/13/2020 7:28 AM, Anton Khirnov wrote:
>>> Those SEIs refer to the currently active SPS. However, since the SEI
>>> NALUs precede the coded picture data in the bitstream, the active SPS is
>>> in general not known when we are decoding the SEI.
>>>
>>> Therefore, store the content of the picture timing SEIs and actually
>>> parse it when the active SPS is known.
>>> ---
>>>  libavcodec/h264_parser.c |  9 +++++
>>>  libavcodec/h264_sei.c    | 86 ++++++++++++++++++++++++----------------
>>>  libavcodec/h264_sei.h    | 11 +++++
>>>  libavcodec/h264_slice.c  | 10 +++++
>>>  4 files changed, 82 insertions(+), 34 deletions(-)
>>>
>>
>> [...]
>>
>>> +static int decode_picture_timing(H264SEIPictureTiming *h, GetBitContext *gb,
>>> +                                 void *logctx)
>>> +{
>>> +    PutBitContext pb;
>>> +    int n;
>>> +
>>> +    if (get_bits_left(gb) > sizeof(h->payload) * 8) {
>>> +        av_log(logctx, AV_LOG_ERROR, "Picture timing SEI payload too large\n");
>>> +        return AVERROR_INVALIDDATA;
>>> +    }
>>> +
>>> +    init_put_bits(&pb, h->payload, sizeof(h->payload));
>>> +
>>> +    while (get_bits_left(gb) >= 32)
>>> +        put_bits(&pb, 32, get_bits_long(gb, 32));
>>> +
>>> +    n = get_bits_left(gb);
>>> +    if (n)
>>> +        put_bits(&pb, n, get_bits_long(gb, n));
>>
>> I think you can do something like avpriv_copy_bits(&pb, gb->buffer,
>> get_bits_left(gb)) instead.
> 
> Right, I wasn't sure if we want to depend on payload start being
> byte-aligned. I suppose it has to be true in valid streams so it should
> be ok.
> 
>> No need to skip the bits in gb afterwards since the GetBitContext struct
>> is exclusive for this SEI starting with the previous patch.
> 
> Not sure which skip you mean here.

avpriv_copy_bits() takes an uint8_t* argument as input, and unlike when
doing put_bits(&pb, 32, get_bits_long(gb, 32)), the GetBitContext will
remain unchanged.
If if was required to keep parsing the following SEI message in the same
context you'd need to do skip_bits_long() on it. But that's not needed
now that each individual message uses it's own context.