diff mbox

[FFmpeg-devel,4/4] lavc/cbs_h2645: fix no slice data trigger the assert.

Message ID 1526015513-17475-4-git-send-email-mypopydev@gmail.com
State New
Headers show

Commit Message

Jun Zhao May 11, 2018, 5:11 a.m. UTC
when the NALU data with zero, just give a warning.

Fixes ticket #7200

Signed-off-by: Jun Zhao <mypopydev@gmail.com>
---
 libavcodec/cbs_h2645.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

James Almer May 11, 2018, 5:31 a.m. UTC | #1
On 5/11/2018 2:11 AM, Jun Zhao wrote:
> when the NALU data with zero, just give a warning.
> 
> Fixes ticket #7200
> 
> Signed-off-by: Jun Zhao <mypopydev@gmail.com>
> ---
>  libavcodec/cbs_h2645.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
> index ab33cdb..08b060c 100644
> --- a/libavcodec/cbs_h2645.c
> +++ b/libavcodec/cbs_h2645.c
> @@ -521,7 +521,11 @@ static int cbs_h2645_fragment_add_nals(CodedBitstreamContext *ctx,
>          // Remove trailing zeroes.
>          while (size > 0 && nal->data[size - 1] == 0)
>              --size;
> -        av_assert0(size > 0);
> +        if (!size) {
> +            av_log(ctx->log_ctx, AV_LOG_WARNING, "No slice data - that was just the header. "
> +                   "Probably invalid unaligned padding on non-final NAL unit.\n");
> +            continue;
> +        }

Maybe ff_h2645_packet_split() should instead just skip the NALU
altogether if after removing all trailing zero bits the size in bytes
ends up being zero?

>  
>          data = av_malloc(size + AV_INPUT_BUFFER_PADDING_SIZE);
>          if (!data)
>
Mark Thompson May 11, 2018, 10:10 a.m. UTC | #2
On 11/05/18 06:11, Jun Zhao wrote:
> when the NALU data with zero, just give a warning.
> 
> Fixes ticket #7200
> 
> Signed-off-by: Jun Zhao <mypopydev@gmail.com>
> ---
>  libavcodec/cbs_h2645.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
> index ab33cdb..08b060c 100644
> --- a/libavcodec/cbs_h2645.c
> +++ b/libavcodec/cbs_h2645.c
> @@ -521,7 +521,11 @@ static int cbs_h2645_fragment_add_nals(CodedBitstreamContext *ctx,
>          // Remove trailing zeroes.
>          while (size > 0 && nal->data[size - 1] == 0)
>              --size;
> -        av_assert0(size > 0);
> +        if (!size) {
> +            av_log(ctx->log_ctx, AV_LOG_WARNING, "No slice data - that was just the header. "
> +                   "Probably invalid unaligned padding on non-final NAL unit.\n");
> +            continue;
> +        }
>  
>          data = av_malloc(size + AV_INPUT_BUFFER_PADDING_SIZE);
>          if (!data)
> 

What do we actually want the result to be here?

On IRC, James suggested:

> diff --git a/libavcodec/h2645_parse.c b/libavcodec/h2645_parse.c
> index dbf2435677..d436d65f48 100644
> --- a/libavcodec/h2645_parse.c
> +++ b/libavcodec/h2645_parse.c
> @@ -371,7 +371,7 @@ int ff_h2645_packet_split(H2645Packet *pkt, const uint8_t *buf, int length,
>              ret = hevc_parse_nal_header(nal, logctx);
>          else
>              ret = h264_parse_nal_header(nal, logctx);
> -        if (ret <= 0 || nal->size <= 0) {
> +        if (ret <= 0 || nal->size <= 0 || nal->size_bits <= 0) {
>              if (ret < 0) {
>                  av_log(logctx, AV_LOG_ERROR, "Invalid NAL unit %d, skipping.\n",
>                         nal->type);

which removes it before it gets to the CBS code.

Another thing we could do is:

> diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
> index ab33cdb69b..46cd887cdd 100644
> --- a/libavcodec/cbs_h2645.c
> +++ b/libavcodec/cbs_h2645.c
> @@ -519,7 +519,7 @@ static int cbs_h2645_fragment_add_nals(CodedBitstreamContext *ctx,
>          uint8_t *data;
>  
>          // Remove trailing zeroes.
> -        while (size > 0 && nal->data[size - 1] == 0)
> +        while (size > 1 && nal->data[size - 1] == 0)
>              --size;
>          av_assert0(size > 0);
>  

which would make it parse as an empty NAL unit of type 0 (unspecified), and therefore pass through into the output stream in the h264_metadata case.

So, what do you think?  Do you know what made your sample stream?

- Mark
James Almer May 11, 2018, 3:38 p.m. UTC | #3
On 5/11/2018 7:10 AM, Mark Thompson wrote:
> On 11/05/18 06:11, Jun Zhao wrote:
>> when the NALU data with zero, just give a warning.
>>
>> Fixes ticket #7200
>>
>> Signed-off-by: Jun Zhao <mypopydev@gmail.com>
>> ---
>>  libavcodec/cbs_h2645.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
>> index ab33cdb..08b060c 100644
>> --- a/libavcodec/cbs_h2645.c
>> +++ b/libavcodec/cbs_h2645.c
>> @@ -521,7 +521,11 @@ static int cbs_h2645_fragment_add_nals(CodedBitstreamContext *ctx,
>>          // Remove trailing zeroes.
>>          while (size > 0 && nal->data[size - 1] == 0)
>>              --size;
>> -        av_assert0(size > 0);
>> +        if (!size) {
>> +            av_log(ctx->log_ctx, AV_LOG_WARNING, "No slice data - that was just the header. "
>> +                   "Probably invalid unaligned padding on non-final NAL unit.\n");
>> +            continue;
>> +        }
>>  
>>          data = av_malloc(size + AV_INPUT_BUFFER_PADDING_SIZE);
>>          if (!data)
>>
> 
> What do we actually want the result to be here?
> 
> On IRC, James suggested:
> 
>> diff --git a/libavcodec/h2645_parse.c b/libavcodec/h2645_parse.c
>> index dbf2435677..d436d65f48 100644
>> --- a/libavcodec/h2645_parse.c
>> +++ b/libavcodec/h2645_parse.c
>> @@ -371,7 +371,7 @@ int ff_h2645_packet_split(H2645Packet *pkt, const uint8_t *buf, int length,
>>              ret = hevc_parse_nal_header(nal, logctx);
>>          else
>>              ret = h264_parse_nal_header(nal, logctx);
>> -        if (ret <= 0 || nal->size <= 0) {
>> +        if (ret <= 0 || nal->size <= 0 || nal->size_bits <= 0) {
>>              if (ret < 0) {
>>                  av_log(logctx, AV_LOG_ERROR, "Invalid NAL unit %d, skipping.\n",
>>                         nal->type);
> 
> which removes it before it gets to the CBS code.
> 
> Another thing we could do is:
> 
>> diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
>> index ab33cdb69b..46cd887cdd 100644
>> --- a/libavcodec/cbs_h2645.c
>> +++ b/libavcodec/cbs_h2645.c
>> @@ -519,7 +519,7 @@ static int cbs_h2645_fragment_add_nals(CodedBitstreamContext *ctx,
>>          uint8_t *data;
>>  
>>          // Remove trailing zeroes.
>> -        while (size > 0 && nal->data[size - 1] == 0)
>> +        while (size > 1 && nal->data[size - 1] == 0)
>>              --size;
>>          av_assert0(size > 0);
>>  
> 
> which would make it parse as an empty NAL unit of type 0 (unspecified), and therefore pass through into the output stream in the h264_metadata case.
> 
> So, what do you think?  Do you know what made your sample stream?
> 
> - Mark

Taking into account the analysis by mkver in the trac ticket, where he
found out the bitstream contains "00 00 00 01 00 00 00 01" with the
second start code being a real valid NAL, i think this should definitely
be fixed in h2645_parse. No reason to propagate a non existent NAL like
this.
We should either use my fix, or another that actually prevents nal->size
from inexplicably becoming 1 in this scenario.
Mark Thompson May 12, 2018, 11:07 p.m. UTC | #4
On 11/05/18 16:38, James Almer wrote:
> On 5/11/2018 7:10 AM, Mark Thompson wrote:
>> On 11/05/18 06:11, Jun Zhao wrote:
>>> when the NALU data with zero, just give a warning.
>>>
>>> Fixes ticket #7200
>>>
>>> Signed-off-by: Jun Zhao <mypopydev@gmail.com>
>>> ---
>>>  libavcodec/cbs_h2645.c | 6 +++++-
>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
>>> index ab33cdb..08b060c 100644
>>> --- a/libavcodec/cbs_h2645.c
>>> +++ b/libavcodec/cbs_h2645.c
>>> @@ -521,7 +521,11 @@ static int cbs_h2645_fragment_add_nals(CodedBitstreamContext *ctx,
>>>          // Remove trailing zeroes.
>>>          while (size > 0 && nal->data[size - 1] == 0)
>>>              --size;
>>> -        av_assert0(size > 0);
>>> +        if (!size) {
>>> +            av_log(ctx->log_ctx, AV_LOG_WARNING, "No slice data - that was just the header. "
>>> +                   "Probably invalid unaligned padding on non-final NAL unit.\n");
>>> +            continue;
>>> +        }
>>>  
>>>          data = av_malloc(size + AV_INPUT_BUFFER_PADDING_SIZE);
>>>          if (!data)
>>>
>>
>> What do we actually want the result to be here?
>>
>> On IRC, James suggested:
>>
>>> diff --git a/libavcodec/h2645_parse.c b/libavcodec/h2645_parse.c
>>> index dbf2435677..d436d65f48 100644
>>> --- a/libavcodec/h2645_parse.c
>>> +++ b/libavcodec/h2645_parse.c
>>> @@ -371,7 +371,7 @@ int ff_h2645_packet_split(H2645Packet *pkt, const uint8_t *buf, int length,
>>>              ret = hevc_parse_nal_header(nal, logctx);
>>>          else
>>>              ret = h264_parse_nal_header(nal, logctx);
>>> -        if (ret <= 0 || nal->size <= 0) {
>>> +        if (ret <= 0 || nal->size <= 0 || nal->size_bits <= 0) {
>>>              if (ret < 0) {
>>>                  av_log(logctx, AV_LOG_ERROR, "Invalid NAL unit %d, skipping.\n",
>>>                         nal->type);
>>
>> which removes it before it gets to the CBS code.
>>
>> Another thing we could do is:
>>
>>> diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
>>> index ab33cdb69b..46cd887cdd 100644
>>> --- a/libavcodec/cbs_h2645.c
>>> +++ b/libavcodec/cbs_h2645.c
>>> @@ -519,7 +519,7 @@ static int cbs_h2645_fragment_add_nals(CodedBitstreamContext *ctx,
>>>          uint8_t *data;
>>>  
>>>          // Remove trailing zeroes.
>>> -        while (size > 0 && nal->data[size - 1] == 0)
>>> +        while (size > 1 && nal->data[size - 1] == 0)
>>>              --size;
>>>          av_assert0(size > 0);
>>>  
>>
>> which would make it parse as an empty NAL unit of type 0 (unspecified), and therefore pass through into the output stream in the h264_metadata case.
>>
>> So, what do you think?  Do you know what made your sample stream?
>>
>> - Mark
> 
> Taking into account the analysis by mkver in the trac ticket, where he
> found out the bitstream contains "00 00 00 01 00 00 00 01" with the
> second start code being a real valid NAL, i think this should definitely
> be fixed in h2645_parse. No reason to propagate a non existent NAL like
> this.
> We should either use my fix, or another that actually prevents nal->size
> from inexplicably becoming 1 in this scenario.

I was applying the standard precisely, which I think ends up with the interpretation:

 00 00 00 01 09 f0 00 00 00 01 00 00 00 01 41 e2 02 56
|  |        |  |  |  |        |  |        |  |
 ^^ zero_byte         ^^^^^^^^ start code
    ^^^^^^^^ start code        ^^ NAL unit header
             ^^ NAL unit header   ^^^^^^^^ start code
                ^^ NAL unit content (AUD)  ^^ NAL unit header
                   ^^ trailing zeroes         ^^^... NAL unit content (slice)

The middle NAL unit has type 0 (unspecified application use) and no content.  I admit that's probably not what was intended here, but currently we do preserve unspecified NAL units and it's not clear that type 0 should necessarily be treated differently to 24-31.

This would be a pretty absurd use, though, so I don't think it really matters.  Given that, I'm fine with any of the possible answers above.

- Mark


(Another data point: the reference decoder segfaults when given the sample stream.)
James Almer May 13, 2018, 4:54 a.m. UTC | #5
On 5/12/2018 8:07 PM, Mark Thompson wrote:
> On 11/05/18 16:38, James Almer wrote:
>> On 5/11/2018 7:10 AM, Mark Thompson wrote:
>>> On 11/05/18 06:11, Jun Zhao wrote:
>>>> when the NALU data with zero, just give a warning.
>>>>
>>>> Fixes ticket #7200
>>>>
>>>> Signed-off-by: Jun Zhao <mypopydev@gmail.com>
>>>> ---
>>>>  libavcodec/cbs_h2645.c | 6 +++++-
>>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
>>>> index ab33cdb..08b060c 100644
>>>> --- a/libavcodec/cbs_h2645.c
>>>> +++ b/libavcodec/cbs_h2645.c
>>>> @@ -521,7 +521,11 @@ static int cbs_h2645_fragment_add_nals(CodedBitstreamContext *ctx,
>>>>          // Remove trailing zeroes.
>>>>          while (size > 0 && nal->data[size - 1] == 0)
>>>>              --size;
>>>> -        av_assert0(size > 0);
>>>> +        if (!size) {
>>>> +            av_log(ctx->log_ctx, AV_LOG_WARNING, "No slice data - that was just the header. "
>>>> +                   "Probably invalid unaligned padding on non-final NAL unit.\n");
>>>> +            continue;
>>>> +        }
>>>>  
>>>>          data = av_malloc(size + AV_INPUT_BUFFER_PADDING_SIZE);
>>>>          if (!data)
>>>>
>>>
>>> What do we actually want the result to be here?
>>>
>>> On IRC, James suggested:
>>>
>>>> diff --git a/libavcodec/h2645_parse.c b/libavcodec/h2645_parse.c
>>>> index dbf2435677..d436d65f48 100644
>>>> --- a/libavcodec/h2645_parse.c
>>>> +++ b/libavcodec/h2645_parse.c
>>>> @@ -371,7 +371,7 @@ int ff_h2645_packet_split(H2645Packet *pkt, const uint8_t *buf, int length,
>>>>              ret = hevc_parse_nal_header(nal, logctx);
>>>>          else
>>>>              ret = h264_parse_nal_header(nal, logctx);
>>>> -        if (ret <= 0 || nal->size <= 0) {
>>>> +        if (ret <= 0 || nal->size <= 0 || nal->size_bits <= 0) {
>>>>              if (ret < 0) {
>>>>                  av_log(logctx, AV_LOG_ERROR, "Invalid NAL unit %d, skipping.\n",
>>>>                         nal->type);
>>>
>>> which removes it before it gets to the CBS code.
>>>
>>> Another thing we could do is:
>>>
>>>> diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
>>>> index ab33cdb69b..46cd887cdd 100644
>>>> --- a/libavcodec/cbs_h2645.c
>>>> +++ b/libavcodec/cbs_h2645.c
>>>> @@ -519,7 +519,7 @@ static int cbs_h2645_fragment_add_nals(CodedBitstreamContext *ctx,
>>>>          uint8_t *data;
>>>>  
>>>>          // Remove trailing zeroes.
>>>> -        while (size > 0 && nal->data[size - 1] == 0)
>>>> +        while (size > 1 && nal->data[size - 1] == 0)
>>>>              --size;
>>>>          av_assert0(size > 0);
>>>>  
>>>
>>> which would make it parse as an empty NAL unit of type 0 (unspecified), and therefore pass through into the output stream in the h264_metadata case.
>>>
>>> So, what do you think?  Do you know what made your sample stream?
>>>
>>> - Mark
>>
>> Taking into account the analysis by mkver in the trac ticket, where he
>> found out the bitstream contains "00 00 00 01 00 00 00 01" with the
>> second start code being a real valid NAL, i think this should definitely
>> be fixed in h2645_parse. No reason to propagate a non existent NAL like
>> this.
>> We should either use my fix, or another that actually prevents nal->size
>> from inexplicably becoming 1 in this scenario.
> 
> I was applying the standard precisely, which I think ends up with the interpretation:
> 
>  00 00 00 01 09 f0 00 00 00 01 00 00 00 01 41 e2 02 56
> |  |        |  |  |  |        |  |        |  |
>  ^^ zero_byte         ^^^^^^^^ start code
>     ^^^^^^^^ start code        ^^ NAL unit header
>              ^^ NAL unit header   ^^^^^^^^ start code
>                 ^^ NAL unit content (AUD)  ^^ NAL unit header
>                    ^^ trailing zeroes         ^^^... NAL unit content (slice)
> 
> The middle NAL unit has type 0 (unspecified application use) and no content.  I admit that's probably not what was intended here, but currently we do preserve unspecified NAL units and it's not clear that type 0 should necessarily be treated differently to 24-31.
> 
> This would be a pretty absurd use, though, so I don't think it really matters.  Given that, I'm fine with any of the possible answers above.

I have the feeling this file is meant to have 00 00 00 01 as start code
for all NALUs, and not 00 00 01 plus a trailing/leading zero byte, so
technically this would be a rogue four byte start code before a valid
NALU, perhaps because whatever muxed this file appends start codes to
what it expects to be NALUs stripped of them, and the slice already had one.

In any case, having no content (nal->size_bits being zero, and therefore
nal->gb being an empty context) is reason enough to drop it, IMO.

> 
> - Mark
> 
> 
> (Another data point: the reference decoder segfaults when given the sample stream.)
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Jun Zhao May 14, 2018, 12:36 a.m. UTC | #6
2018-05-11 18:10 GMT+08:00 Mark Thompson <sw@jkqxz.net>:
> On 11/05/18 06:11, Jun Zhao wrote:
>> when the NALU data with zero, just give a warning.
>>
>> Fixes ticket #7200
>>
>> Signed-off-by: Jun Zhao <mypopydev@gmail.com>
>> ---
>>  libavcodec/cbs_h2645.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
>> index ab33cdb..08b060c 100644
>> --- a/libavcodec/cbs_h2645.c
>> +++ b/libavcodec/cbs_h2645.c
>> @@ -521,7 +521,11 @@ static int cbs_h2645_fragment_add_nals(CodedBitstreamContext *ctx,
>>          // Remove trailing zeroes.
>>          while (size > 0 && nal->data[size - 1] == 0)
>>              --size;
>> -        av_assert0(size > 0);
>> +        if (!size) {
>> +            av_log(ctx->log_ctx, AV_LOG_WARNING, "No slice data - that was just the header. "
>> +                   "Probably invalid unaligned padding on non-final NAL unit.\n");
>> +            continue;
>> +        }
>>
>>          data = av_malloc(size + AV_INPUT_BUFFER_PADDING_SIZE);
>>          if (!data)
>>
>
> What do we actually want the result to be here?
>
> On IRC, James suggested:
>
>> diff --git a/libavcodec/h2645_parse.c b/libavcodec/h2645_parse.c
>> index dbf2435677..d436d65f48 100644
>> --- a/libavcodec/h2645_parse.c
>> +++ b/libavcodec/h2645_parse.c
>> @@ -371,7 +371,7 @@ int ff_h2645_packet_split(H2645Packet *pkt, const uint8_t *buf, int length,
>>              ret = hevc_parse_nal_header(nal, logctx);
>>          else
>>              ret = h264_parse_nal_header(nal, logctx);
>> -        if (ret <= 0 || nal->size <= 0) {
>> +        if (ret <= 0 || nal->size <= 0 || nal->size_bits <= 0) {
>>              if (ret < 0) {
>>                  av_log(logctx, AV_LOG_ERROR, "Invalid NAL unit %d, skipping.\n",
>>                         nal->type);
>
> which removes it before it gets to the CBS code.
I agree this way, I think we need drop the invalid NAL unit as early
as possible.
>
> Another thing we could do is:
>
>> diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
>> index ab33cdb69b..46cd887cdd 100644
>> --- a/libavcodec/cbs_h2645.c
>> +++ b/libavcodec/cbs_h2645.c
>> @@ -519,7 +519,7 @@ static int cbs_h2645_fragment_add_nals(CodedBitstreamContext *ctx,
>>          uint8_t *data;
>>
>>          // Remove trailing zeroes.
>> -        while (size > 0 && nal->data[size - 1] == 0)
>> +        while (size > 1 && nal->data[size - 1] == 0)
>>              --size;
>>          av_assert0(size > 0);
>>
>
> which would make it parse as an empty NAL unit of type 0 (unspecified), and therefore pass through into the output stream in the h264_metadata case.
>
> So, what do you think?  Do you know what made your sample stream?
I got this sample from a live streaming  android phone APPs
>
> - Mark
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
Jun Zhao May 14, 2018, 12:45 a.m. UTC | #7
2018-05-11 23:38 GMT+08:00 James Almer <jamrial@gmail.com>:
> On 5/11/2018 7:10 AM, Mark Thompson wrote:
>> On 11/05/18 06:11, Jun Zhao wrote:
>>> when the NALU data with zero, just give a warning.
>>>
>>> Fixes ticket #7200
>>>
>>> Signed-off-by: Jun Zhao <mypopydev@gmail.com>
>>> ---
>>>  libavcodec/cbs_h2645.c | 6 +++++-
>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
>>> index ab33cdb..08b060c 100644
>>> --- a/libavcodec/cbs_h2645.c
>>> +++ b/libavcodec/cbs_h2645.c
>>> @@ -521,7 +521,11 @@ static int cbs_h2645_fragment_add_nals(CodedBitstreamContext *ctx,
>>>          // Remove trailing zeroes.
>>>          while (size > 0 && nal->data[size - 1] == 0)
>>>              --size;
>>> -        av_assert0(size > 0);
>>> +        if (!size) {
>>> +            av_log(ctx->log_ctx, AV_LOG_WARNING, "No slice data - that was just the header. "
>>> +                   "Probably invalid unaligned padding on non-final NAL unit.\n");
>>> +            continue;
>>> +        }
>>>
>>>          data = av_malloc(size + AV_INPUT_BUFFER_PADDING_SIZE);
>>>          if (!data)
>>>
>>
>> What do we actually want the result to be here?
>>
>> On IRC, James suggested:
>>
>>> diff --git a/libavcodec/h2645_parse.c b/libavcodec/h2645_parse.c
>>> index dbf2435677..d436d65f48 100644
>>> --- a/libavcodec/h2645_parse.c
>>> +++ b/libavcodec/h2645_parse.c
>>> @@ -371,7 +371,7 @@ int ff_h2645_packet_split(H2645Packet *pkt, const uint8_t *buf, int length,
>>>              ret = hevc_parse_nal_header(nal, logctx);
>>>          else
>>>              ret = h264_parse_nal_header(nal, logctx);
>>> -        if (ret <= 0 || nal->size <= 0) {
>>> +        if (ret <= 0 || nal->size <= 0 || nal->size_bits <= 0) {
>>>              if (ret < 0) {
>>>                  av_log(logctx, AV_LOG_ERROR, "Invalid NAL unit %d, skipping.\n",
>>>                         nal->type);
>>
>> which removes it before it gets to the CBS code.
>>
>> Another thing we could do is:
>>
>>> diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
>>> index ab33cdb69b..46cd887cdd 100644
>>> --- a/libavcodec/cbs_h2645.c
>>> +++ b/libavcodec/cbs_h2645.c
>>> @@ -519,7 +519,7 @@ static int cbs_h2645_fragment_add_nals(CodedBitstreamContext *ctx,
>>>          uint8_t *data;
>>>
>>>          // Remove trailing zeroes.
>>> -        while (size > 0 && nal->data[size - 1] == 0)
>>> +        while (size > 1 && nal->data[size - 1] == 0)
>>>              --size;
>>>          av_assert0(size > 0);
>>>
>>
>> which would make it parse as an empty NAL unit of type 0 (unspecified), and therefore pass through into the output stream in the h264_metadata case.
>>
>> So, what do you think?  Do you know what made your sample stream?
>>
>> - Mark
>
> Taking into account the analysis by mkver in the trac ticket, where he
> found out the bitstream contains "00 00 00 01 00 00 00 01" with the
> second start code being a real valid NAL, i think this should definitely
> be fixed in h2645_parse. No reason to propagate a non existent NAL like
> this.
> We should either use my fix, or another that actually prevents nal->size
> from inexplicably becoming 1 in this scenario.
Now h2645_parse give a loose check in ff_h2645_packet_split(), and I agree
use your fix
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
diff mbox

Patch

diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
index ab33cdb..08b060c 100644
--- a/libavcodec/cbs_h2645.c
+++ b/libavcodec/cbs_h2645.c
@@ -521,7 +521,11 @@  static int cbs_h2645_fragment_add_nals(CodedBitstreamContext *ctx,
         // Remove trailing zeroes.
         while (size > 0 && nal->data[size - 1] == 0)
             --size;
-        av_assert0(size > 0);
+        if (!size) {
+            av_log(ctx->log_ctx, AV_LOG_WARNING, "No slice data - that was just the header. "
+                   "Probably invalid unaligned padding on non-final NAL unit.\n");
+            continue;
+        }
 
         data = av_malloc(size + AV_INPUT_BUFFER_PADDING_SIZE);
         if (!data)