diff mbox series

[FFmpeg-devel] avformat/hlsenc: fix CODECS attribute of H.264

Message ID tencent_42C1CFBE55722700B8A0160A4D11207DCD07@qq.com
State New
Headers show
Series [FFmpeg-devel] avformat/hlsenc: fix CODECS attribute of H.264 | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Zhao Zhili May 29, 2023, 10:16 p.m. UTC 
From: Zhao Zhili <zhilizhao@tencent.com>

Signed-off-by: Zhao Zhili <zhilizhao@tencent.com>
---
 libavformat/hlsenc.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

Comments

Anton Khirnov May 29, 2023, 2:46 p.m. UTC | #1 
Quoting Zhao Zhili (2023-05-30 00:16:05)
>Subject: Re: [FFmpeg-devel] [PATCH] avformat/hlsenc: fix CODECS attribute of H.264

"fix bug" commit message are not very useful
Zhao Zhili May 29, 2023, 3:08 p.m. UTC | #2 
> 在 2023年5月29日，22:46，Anton Khirnov <anton@khirnov.net> 写道：
> 
> Quoting Zhao Zhili (2023-05-30 00:16:05)
>> Subject: Re: [FFmpeg-devel] [PATCH] avformat/hlsenc: fix CODECS attribute of H.264
> 
> "fix bug" commit message are not very useful

However, CODECS attribute is a particular detail for HLS. Any suggestions to make it more clear and not too verbose?

> 
> -- 
> Anton Khirnov
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
Anton Khirnov May 29, 2023, 3:16 p.m. UTC | #3 
Quoting Zhao Zhili (2023-05-29 17:08:50)
> 
> > 在 2023年5月29日，22:46，Anton Khirnov <anton@khirnov.net> 写道：
> > 
> > Quoting Zhao Zhili (2023-05-30 00:16:05)
> >> Subject: Re: [FFmpeg-devel] [PATCH] avformat/hlsenc: fix CODECS attribute of H.264
> > 
> > "fix bug" commit message are not very useful
> 
> However, CODECS attribute is a particular detail for HLS. Any suggestions to make it more clear and not too verbose?

Keep the first line as is, but add a commit message body that says at
least what the problem was. Possibly also how it's being fixed, if it is
not obvious.
Lance Wang May 30, 2023, 1:03 a.m. UTC | #4 
On Mon, May 29, 2023 at 10:16 PM Zhao Zhili <quinkblack@foxmail.com> wrote:

> From: Zhao Zhili <zhilizhao@tencent.com>
>
> Signed-off-by: Zhao Zhili <zhilizhao@tencent.com>
> ---
>  libavformat/hlsenc.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
> index 871afb571b..1e0848ce3d 100644
> --- a/libavformat/hlsenc.c
> +++ b/libavformat/hlsenc.c
> @@ -355,9 +355,19 @@ static void write_codec_attr(AVStream *st,
> VariantStream *vs)
>
>      if (st->codecpar->codec_id == AV_CODEC_ID_H264) {
>          uint8_t *data = st->codecpar->extradata;
> -        if (data && (data[0] | data[1] | data[2]) == 0 && data[3] == 1 &&
> (data[4] & 0x1F) == 7) {
> +        if (data) {
> +            const uint8_t *p;
> +
> +            if (AV_RB32(data) == 0x01 && (data[4] & 0x1F) == 7)
> +                p = &data[5];
> +            else if (AV_RB24(data) == 0x01 && (data[3] & 0x1F) == 7)
> +                p = &data[4];
> +            else if (data[0] == 0x01)  /* avcC */
> +                p = &data[1];
> +            else
> +                goto fail;
>

how to reproduce the issue? I recall mpegts is annex b format and sps/pps
start code is 4 byte always.


>              snprintf(attr, sizeof(attr),
> -                     "avc1.%02x%02x%02x", data[5], data[6], data[7]);
> +                     "avc1.%02x%02x%02x", p[0], p[1], p[2]);
>          } else {
>              goto fail;
>          }
> --
> 2.25.1
>
> _______________________________________________
> 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".
>
Zhao Zhili May 30, 2023, 3:09 a.m. UTC | #5 
> On May 30, 2023, at 09:03, Lance Wang <lance.lmwang@gmail.com> wrote:
> 
> On Mon, May 29, 2023 at 10:16 PM Zhao Zhili <quinkblack@foxmail.com> wrote:
> 
>> From: Zhao Zhili <zhilizhao@tencent.com>
>> 
>> Signed-off-by: Zhao Zhili <zhilizhao@tencent.com>
>> ---
>> libavformat/hlsenc.c | 14 ++++++++++++--
>> 1 file changed, 12 insertions(+), 2 deletions(-)
>> 
>> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
>> index 871afb571b..1e0848ce3d 100644
>> --- a/libavformat/hlsenc.c
>> +++ b/libavformat/hlsenc.c
>> @@ -355,9 +355,19 @@ static void write_codec_attr(AVStream *st,
>> VariantStream *vs)
>> 
>>     if (st->codecpar->codec_id == AV_CODEC_ID_H264) {
>>         uint8_t *data = st->codecpar->extradata;
>> -        if (data && (data[0] | data[1] | data[2]) == 0 && data[3] == 1 &&
>> (data[4] & 0x1F) == 7) {
>> +        if (data) {
>> +            const uint8_t *p;
>> +
>> +            if (AV_RB32(data) == 0x01 && (data[4] & 0x1F) == 7)
>> +                p = &data[5];
>> +            else if (AV_RB24(data) == 0x01 && (data[3] & 0x1F) == 7)
>> +                p = &data[4];
>> +            else if (data[0] == 0x01)  /* avcC */
>> +                p = &data[1];
>> +            else
>> +                goto fail;
>> 
> 
> how to reproduce the issue? I recall mpegts is annex b format and sps/pps
> start code is 4 byte always.

For example:

ffmpeg -i foo.mp4 -c copy -hls_playlist_type vod -master_pl_name bar.m3u8 test.m3u8

1. The input of hls muxer can be avcc

2. The output of hls muxer can be avcc too, with fmp4 segments.

3. Start code of SPS/PPS should be 0 0 0 1, however, 0 0 1 exist in wild.
ff_isom_write_avcc() does the same:

    /* check for H.264 start code */
    if (AV_RB32(data) != 0x00000001 &&
        AV_RB24(data) != 0x000001) {
        avio_write(pb, data, len);
        return 0;
    }

ff_isom_write_avcc() can be used here, and get/create CODECS attribute can be
shared by multiple muxers. I won't go that further by now.


> 
> 
>>             snprintf(attr, sizeof(attr),
>> -                     "avc1.%02x%02x%02x", data[5], data[6], data[7]);
>> +                     "avc1.%02x%02x%02x", p[0], p[1], p[2]);
>>         } else {
>>             goto fail;
>>         }
>> --
>> 2.25.1
>> 
>> _______________________________________________
>> 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".
>> 
> _______________________________________________
> 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".
Lance Wang May 30, 2023, 4:44 a.m. UTC | #6 
On Tue, May 30, 2023 at 11:09 AM "zhilizhao(赵志立)" <quinkblack@foxmail.com>
wrote:

>
>
> > On May 30, 2023, at 09:03, Lance Wang <lance.lmwang@gmail.com> wrote:
> >
> > On Mon, May 29, 2023 at 10:16 PM Zhao Zhili <quinkblack@foxmail.com>
> wrote:
> >
> >> From: Zhao Zhili <zhilizhao@tencent.com>
> >>
> >> Signed-off-by: Zhao Zhili <zhilizhao@tencent.com>
> >> ---
> >> libavformat/hlsenc.c | 14 ++++++++++++--
> >> 1 file changed, 12 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
> >> index 871afb571b..1e0848ce3d 100644
> >> --- a/libavformat/hlsenc.c
> >> +++ b/libavformat/hlsenc.c
> >> @@ -355,9 +355,19 @@ static void write_codec_attr(AVStream *st,
> >> VariantStream *vs)
> >>
> >>     if (st->codecpar->codec_id == AV_CODEC_ID_H264) {
> >>         uint8_t *data = st->codecpar->extradata;
> >> -        if (data && (data[0] | data[1] | data[2]) == 0 && data[3] == 1
> &&
> >> (data[4] & 0x1F) == 7) {
> >> +        if (data) {
> >> +            const uint8_t *p;
> >> +
> >> +            if (AV_RB32(data) == 0x01 && (data[4] & 0x1F) == 7)
> >> +                p = &data[5];
> >> +            else if (AV_RB24(data) == 0x01 && (data[3] & 0x1F) == 7)
> >> +                p = &data[4];
> >> +            else if (data[0] == 0x01)  /* avcC */
> >> +                p = &data[1];
> >> +            else
> >> +                goto fail;
> >>
> >
> > how to reproduce the issue? I recall mpegts is annex b format and sps/pps
> > start code is 4 byte always.
>
> For example:
>
> ffmpeg -i foo.mp4 -c copy -hls_playlist_type vod -master_pl_name bar.m3u8
> test.m3u8
>
> 1. The input of hls muxer can be avcc
>
> 2. The output of hls muxer can be avcc too, with fmp4 segments.
>
>
Yes, this is the case. I had to add mp4toannexb bsf after the copy for avcc
input.  If we
don't add the format conversion, hevc may  have the same issue I guess.
Also we should
check the size of extradata.



> 3. Start code of SPS/PPS should be 0 0 0 1, however, 0 0 1 exist in wild.
> ff_isom_write_avcc() does the same:
>
>     /* check for H.264 start code */
>     if (AV_RB32(data) != 0x00000001 &&
>         AV_RB24(data) != 0x000001) {
>         avio_write(pb, data, len);
>         return 0;
>     }
>
> ff_isom_write_avcc() can be used here, and get/create CODECS attribute can
> be
> shared by multiple muxers. I won't go that further by now.
>
>
The next NALU checking is for SPS only, so one more byte is zero_byte. It's
required by specs:

zero_byte is a single byte equal to 0x00.

When any of the following conditions are true, the zero_byte syntax element
shall be present:

   -

   –  the nal_unit_type within the nal_unit( ) is equal to 7 (sequence
   parameter set) or 8 (picture parameter set),
   -

   –  the byte stream NAL unit syntax structure contains the first NAL unit
   of an access unit in decoding order, as specified in clause 7.4.1.2.3.





>
> >
> >
> >>             snprintf(attr, sizeof(attr),
> >> -                     "avc1.%02x%02x%02x", data[5], data[6], data[7]);
> >> +                     "avc1.%02x%02x%02x", p[0], p[1], p[2]);
> >>         } else {
> >>             goto fail;
> >>         }
> >> --
> >> 2.25.1
> >>
> >> _______________________________________________
> >> 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".
> >>
> > _______________________________________________
> > 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".
>
> _______________________________________________
> 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".
>
Zhao Zhili May 30, 2023, 5:28 a.m. UTC | #7 
> On May 30, 2023, at 12:44, Lance Wang <lance.lmwang@gmail.com> wrote:
> 
> On Tue, May 30, 2023 at 11:09 AM "zhilizhao(赵志立)" <quinkblack@foxmail.com>
> wrote:
> 
>> 
>> 
>>> On May 30, 2023, at 09:03, Lance Wang <lance.lmwang@gmail.com> wrote:
>>> 
>>> On Mon, May 29, 2023 at 10:16 PM Zhao Zhili <quinkblack@foxmail.com>
>> wrote:
>>> 
>>>> From: Zhao Zhili <zhilizhao@tencent.com>
>>>> 
>>>> Signed-off-by: Zhao Zhili <zhilizhao@tencent.com>
>>>> ---
>>>> libavformat/hlsenc.c | 14 ++++++++++++--
>>>> 1 file changed, 12 insertions(+), 2 deletions(-)
>>>> 
>>>> diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
>>>> index 871afb571b..1e0848ce3d 100644
>>>> --- a/libavformat/hlsenc.c
>>>> +++ b/libavformat/hlsenc.c
>>>> @@ -355,9 +355,19 @@ static void write_codec_attr(AVStream *st,
>>>> VariantStream *vs)
>>>> 
>>>>    if (st->codecpar->codec_id == AV_CODEC_ID_H264) {
>>>>        uint8_t *data = st->codecpar->extradata;
>>>> -        if (data && (data[0] | data[1] | data[2]) == 0 && data[3] == 1
>> &&
>>>> (data[4] & 0x1F) == 7) {
>>>> +        if (data) {
>>>> +            const uint8_t *p;
>>>> +
>>>> +            if (AV_RB32(data) == 0x01 && (data[4] & 0x1F) == 7)
>>>> +                p = &data[5];
>>>> +            else if (AV_RB24(data) == 0x01 && (data[3] & 0x1F) == 7)
>>>> +                p = &data[4];
>>>> +            else if (data[0] == 0x01)  /* avcC */
>>>> +                p = &data[1];
>>>> +            else
>>>> +                goto fail;
>>>> 
>>> 
>>> how to reproduce the issue? I recall mpegts is annex b format and sps/pps
>>> start code is 4 byte always.
>> 
>> For example:
>> 
>> ffmpeg -i foo.mp4 -c copy -hls_playlist_type vod -master_pl_name bar.m3u8
>> test.m3u8
>> 
>> 1. The input of hls muxer can be avcc
>> 
>> 2. The output of hls muxer can be avcc too, with fmp4 segments.
>> 
>> 
> Yes, this is the case. I had to add mp4toannexb bsf after the copy for avcc
> input.  If we
> don't add the format conversion, hevc may  have the same issue I guess.

Use mp4toannexb for TS segments is a good practice, but not for
fmp4.

No comments on hevc.

> Also we should
> check the size of extradata.

extradata has a AV_INPUT_BUFFER_PADDING_SIZE, so not necessary.

> 
> 
> 
>> 3. Start code of SPS/PPS should be 0 0 0 1, however, 0 0 1 exist in wild.
>> ff_isom_write_avcc() does the same:
>> 
>>    /* check for H.264 start code */
>>    if (AV_RB32(data) != 0x00000001 &&
>>        AV_RB24(data) != 0x000001) {
>>        avio_write(pb, data, len);
>>        return 0;
>>    }
>> 
>> ff_isom_write_avcc() can be used here, and get/create CODECS attribute can
>> be
>> shared by multiple muxers. I won't go that further by now.
>> 
>> 
> The next NALU checking is for SPS only, so one more byte is zero_byte. It's
> required by specs:
> 
> zero_byte is a single byte equal to 0x00.
> 
> When any of the following conditions are true, the zero_byte syntax element
> shall be present:
> 
>   -
> 
>   –  the nal_unit_type within the nal_unit( ) is equal to 7 (sequence
>   parameter set) or 8 (picture parameter set),
>   -
> 
>   –  the byte stream NAL unit syntax structure contains the first NAL unit
>   of an access unit in decoding order, as specified in clause 7.4.1.2.3.

Yes, but it’s not the first time people don’t follow spec.


> 
> 
> 
>> 
>>> 
>>> 
>>>>            snprintf(attr, sizeof(attr),
>>>> -                     "avc1.%02x%02x%02x", data[5], data[6], data[7]);
>>>> +                     "avc1.%02x%02x%02x", p[0], p[1], p[2]);
>>>>        } else {
>>>>            goto fail;
>>>>        }
>>>> --
>>>> 2.25.1
>>>> 
>>>> _______________________________________________
>>>> 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".
>>>> 
>>> _______________________________________________
>>> 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".
>> 
>> _______________________________________________
>> 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".
>> 
> _______________________________________________
> 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".
diff mbox series

Patch

diff --git a/libavformat/hlsenc.c b/libavformat/hlsenc.c
index 871afb571b..1e0848ce3d 100644
--- a/libavformat/hlsenc.c
+++ b/libavformat/hlsenc.c
@@ -355,9 +355,19 @@  static void write_codec_attr(AVStream *st, VariantStream *vs)
 
     if (st->codecpar->codec_id == AV_CODEC_ID_H264) {
         uint8_t *data = st->codecpar->extradata;
-        if (data && (data[0] | data[1] | data[2]) == 0 && data[3] == 1 && (data[4] & 0x1F) == 7) {
+        if (data) {
+            const uint8_t *p;
+
+            if (AV_RB32(data) == 0x01 && (data[4] & 0x1F) == 7)
+                p = &data[5];
+            else if (AV_RB24(data) == 0x01 && (data[3] & 0x1F) == 7)
+                p = &data[4];
+            else if (data[0] == 0x01)  /* avcC */
+                p = &data[1];
+            else
+                goto fail;
             snprintf(attr, sizeof(attr),
-                     "avc1.%02x%02x%02x", data[5], data[6], data[7]);
+                     "avc1.%02x%02x%02x", p[0], p[1], p[2]);
         } else {
             goto fail;
         }