[FFmpeg-devel] [PATCH] lavc/hevc: Allow arbitrarily many trailing_zero_8bits after a NAL unit in bytestream format.

[Mark Thompson]

On 17/03/16 07:51, Hendrik Leppkes wrote:
> On Thu, Mar 17, 2016 at 12:50 AM, Mark Thompson <sw at jkqxz.net> wrote:
>> On 16/03/16 23:41, Hendrik Leppkes wrote:
>>> On Wed, Mar 16, 2016 at 9:37 PM, Mark Thompson <sw at jkqxz.net> wrote:
>>>> ---
>>>> libavcodec/hevc_parse.c | 14 ++++++++++++++
>>>> 1 file changed, 14 insertions(+)
>>>>
>>>> diff --git a/libavcodec/hevc_parse.c b/libavcodec/hevc_parse.c
>>>> index 63ed84a..8c629ff 100644
>>>> --- a/libavcodec/hevc_parse.c
>>>> +++ b/libavcodec/hevc_parse.c
>>>> @@ -227,6 +227,20 @@ int ff_hevc_split_packet(HEVCContext *s, HEVCPacket *pkt, const uint8_t *buf, in
>>>>                 return AVERROR_INVALIDDATA;
>>>>             }
>>>>         } else {
>>>> +            if (pkt->nals > 0) {
>>>> +                // Discard arbtrarily many trailing_zero_8bits before the
>>>> +                // start code of the next NAL unit.
>>>> +                while (buf[0] == 0 && buf[1] == 0 && buf[2] == 0) {
>>>> +                    ++buf;
>>>> +                    --length;
>>>> +                    if (length < 4)
>>>> +                        break;
>>>> +                }
>>>> +                if (length < 4) {
>>>> +                    // There are only zeroes left, so no more NAL units here.
>>>> +                    break;
>>>> +                }
>>>> +            }
>>>>             /* search start code */
>>>>             while (buf[0] != 0 || buf[1] != 0 || buf[2] != 1) {
>>>>                 ++buf;
>>>
>>> I'm slightly confused, wouldn't the loop right after skip over the
>>> zeroes until it finds a valid start code?
>>> It essentially does the same on a cursory look.
>>
>> Yes - it doesn't change the scanning process, but it does change the return value because in the current code we return failure (and give up on the packet entirely) if there aren't any more NAL units to be found.
>>
>> We actually want to return success if we already have at least one NAL unit and then there are zeroes to the end of the packet.
>>
> 
> Wouldn't it be simpler then to add a condition to the existing loop to
> only error out when no NALs exist?

I apologise if I'm not understanding exactly what you mean, but wouldn't that
then accept any trailing garbage at all as long as there is one valid NAL unit?

I was trying to make the minimum change to additionally accept all streams with
trailing zero bytes like the one in the bug report.  A broader modification like
that would also work, but it's not obvious (to me) that it would be a positive
change overall.  (Though admittedly it does already accept up to three bytes of
trailing garbage because of the < 4 test.)

> PS: You probably want to check pkt->nb_nals, not pkt->nals, the second
> is a memory buffer.

Yep, thanks for catching that.  (And somewhat disappointed that the compiler
didn't - seems to be in Wextra but not Wall for gcc.)

- Mark