diff mbox series

[FFmpeg-devel,1/5] avcodec/h264_mp4toannexb_bsf: refactor start_code_size handling

Message ID tencent_14EA9C130B1C5C0BB526255088E6C967D806@qq.com
State New
Headers show
Series [FFmpeg-devel,1/5] avcodec/h264_mp4toannexb_bsf: refactor start_code_size handling | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished

Commit Message

Zhao Zhili May 19, 2023, 4:41 p.m. UTC
From: Zhao Zhili <zhilizhao@tencent.com>

start_code_size depends on whether PS comes from out-of-band or
in-band. Make the code more readable.
---
 libavcodec/h264_mp4toannexb_bsf.c | 34 ++++++++++++++++++++++++-------
 1 file changed, 27 insertions(+), 7 deletions(-)

Comments

Zhao Zhili May 29, 2023, 12:31 p.m. UTC | #1
Ping for the patchset.

> On May 20, 2023, at 00:41, Zhao Zhili <quinkblack@foxmail.com> wrote:
> 
> From: Zhao Zhili <zhilizhao@tencent.com>
> 
> start_code_size depends on whether PS comes from out-of-band or
> in-band. Make the code more readable.
> ---
> libavcodec/h264_mp4toannexb_bsf.c | 34 ++++++++++++++++++++++++-------
> 1 file changed, 27 insertions(+), 7 deletions(-)
> 
> diff --git a/libavcodec/h264_mp4toannexb_bsf.c b/libavcodec/h264_mp4toannexb_bsf.c
> index d11be455c2..7dce1ae9b6 100644
> --- a/libavcodec/h264_mp4toannexb_bsf.c
> +++ b/libavcodec/h264_mp4toannexb_bsf.c
> @@ -43,10 +43,26 @@ typedef struct H264BSFContext {
>     int      extradata_parsed;
> } H264BSFContext;
> 
> +enum PsSource {
> +    PS_OUT_OF_BAND = -1,
> +    PS_NONE = 0,
> +    PS_IN_BAND = 1,
> +};
> +
> static void count_or_copy(uint8_t **out, uint64_t *out_size,
> -                          const uint8_t *in, int in_size, int ps, int copy)
> +                          const uint8_t *in, int in_size, enum PsSource ps, int copy)
> {
> -    uint8_t start_code_size = ps < 0 ? 0 : *out_size == 0 || ps ? 4 : 3;
> +    uint8_t start_code_size;
> +
> +    if (ps == PS_OUT_OF_BAND)
> +        /* start code already present in out-of-band ps data, so don't need to
> +         * add it manually again
> +         */
> +        start_code_size = 0;
> +    else if (ps == PS_IN_BAND || *out_size == 0)
> +        start_code_size = 4;
> +    else
> +        start_code_size = 3;
> 
>     if (copy) {
>         memcpy(*out + start_code_size, in, in_size);
> @@ -202,6 +218,7 @@ static int h264_mp4toannexb_filter(AVBSFContext *ctx, AVPacket *opkt)
> 
>         do {
>             uint32_t nal_size = 0;
> +            enum PsSource ps;
> 
>             /* possible overread ok due to padding */
>             for (int i = 0; i < s->length_size; i++)
> @@ -230,7 +247,7 @@ static int h264_mp4toannexb_filter(AVBSFContext *ctx, AVPacket *opkt)
>                     if (!s->sps_size) {
>                         LOG_ONCE(ctx, AV_LOG_WARNING, "SPS not present in the stream, nor in AVCC, stream may be unreadable\n");
>                     } else {
> -                        count_or_copy(&out, &out_size, s->sps, s->sps_size, -1, j);
> +                        count_or_copy(&out, &out_size, s->sps, s->sps_size, PS_OUT_OF_BAND, j);
>                         sps_seen = 1;
>                     }
>                 }
> @@ -246,19 +263,22 @@ static int h264_mp4toannexb_filter(AVBSFContext *ctx, AVPacket *opkt)
>             if (new_idr && unit_type == H264_NAL_IDR_SLICE && !sps_seen && !pps_seen) {
>                 if (ctx->par_out->extradata)
>                     count_or_copy(&out, &out_size, ctx->par_out->extradata,
> -                                  ctx->par_out->extradata_size, -1, j);
> +                                  ctx->par_out->extradata_size, PS_OUT_OF_BAND, j);
>                 new_idr = 0;
>             /* if only SPS has been seen, also insert PPS */
>             } else if (new_idr && unit_type == H264_NAL_IDR_SLICE && sps_seen && !pps_seen) {
>                 if (!s->pps_size) {
>                     LOG_ONCE(ctx, AV_LOG_WARNING, "PPS not present in the stream, nor in AVCC, stream may be unreadable\n");
>                 } else {
> -                    count_or_copy(&out, &out_size, s->pps, s->pps_size, -1, j);
> +                    count_or_copy(&out, &out_size, s->pps, s->pps_size, PS_OUT_OF_BAND, j);
>                 }
>             }
> 
> -            count_or_copy(&out, &out_size, buf, nal_size,
> -                          unit_type == H264_NAL_SPS || unit_type == H264_NAL_PPS, j);
> +            if (unit_type == H264_NAL_SPS || unit_type == H264_NAL_PPS)
> +                ps = PS_IN_BAND;
> +            else
> +                ps = PS_NONE;
> +            count_or_copy(&out, &out_size, buf, nal_size, ps, j);
>             if (!new_idr && unit_type == H264_NAL_SLICE) {
>                 new_idr  = 1;
>                 sps_seen = 0;
> -- 
> 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".
Jun Zhao May 29, 2023, 3:54 p.m. UTC | #2
On Fri, May 19, 2023 at 4:41 PM Zhao Zhili <quinkblack@foxmail.com> wrote:
>
> From: Zhao Zhili <zhilizhao@tencent.com>
>
> start_code_size depends on whether PS comes from out-of-band or
> in-band. Make the code more readable.
> ---
>  libavcodec/h264_mp4toannexb_bsf.c | 34 ++++++++++++++++++++++++-------
>  1 file changed, 27 insertions(+), 7 deletions(-)
>
> diff --git a/libavcodec/h264_mp4toannexb_bsf.c b/libavcodec/h264_mp4toannexb_bsf.c
> index d11be455c2..7dce1ae9b6 100644
> --- a/libavcodec/h264_mp4toannexb_bsf.c
> +++ b/libavcodec/h264_mp4toannexb_bsf.c
> @@ -43,10 +43,26 @@ typedef struct H264BSFContext {
>      int      extradata_parsed;
>  } H264BSFContext;
>
> +enum PsSource {
> +    PS_OUT_OF_BAND = -1,
> +    PS_NONE = 0,
> +    PS_IN_BAND = 1,
> +};
> +
>  static void count_or_copy(uint8_t **out, uint64_t *out_size,
> -                          const uint8_t *in, int in_size, int ps, int copy)
> +                          const uint8_t *in, int in_size, enum PsSource ps, int copy)
>  {
> -    uint8_t start_code_size = ps < 0 ? 0 : *out_size == 0 || ps ? 4 : 3;
> +    uint8_t start_code_size;
> +
> +    if (ps == PS_OUT_OF_BAND)
> +        /* start code already present in out-of-band ps data, so don't need to
> +         * add it manually again
> +         */
> +        start_code_size = 0;
> +    else if (ps == PS_IN_BAND || *out_size == 0)
> +        start_code_size = 4;
> +    else
> +        start_code_size = 3;
I prefer the original style, they are shorter and more concise
>
>      if (copy) {
>          memcpy(*out + start_code_size, in, in_size);
> @@ -202,6 +218,7 @@ static int h264_mp4toannexb_filter(AVBSFContext *ctx, AVPacket *opkt)
>
>          do {
>              uint32_t nal_size = 0;
> +            enum PsSource ps;
>
>              /* possible overread ok due to padding */
>              for (int i = 0; i < s->length_size; i++)
> @@ -230,7 +247,7 @@ static int h264_mp4toannexb_filter(AVBSFContext *ctx, AVPacket *opkt)
>                      if (!s->sps_size) {
>                          LOG_ONCE(ctx, AV_LOG_WARNING, "SPS not present in the stream, nor in AVCC, stream may be unreadable\n");
>                      } else {
> -                        count_or_copy(&out, &out_size, s->sps, s->sps_size, -1, j);
> +                        count_or_copy(&out, &out_size, s->sps, s->sps_size, PS_OUT_OF_BAND, j);
>                          sps_seen = 1;
>                      }
>                  }
> @@ -246,19 +263,22 @@ static int h264_mp4toannexb_filter(AVBSFContext *ctx, AVPacket *opkt)
>              if (new_idr && unit_type == H264_NAL_IDR_SLICE && !sps_seen && !pps_seen) {
>                  if (ctx->par_out->extradata)
>                      count_or_copy(&out, &out_size, ctx->par_out->extradata,
> -                                  ctx->par_out->extradata_size, -1, j);
> +                                  ctx->par_out->extradata_size, PS_OUT_OF_BAND, j);
>                  new_idr = 0;
>              /* if only SPS has been seen, also insert PPS */
>              } else if (new_idr && unit_type == H264_NAL_IDR_SLICE && sps_seen && !pps_seen) {
>                  if (!s->pps_size) {
>                      LOG_ONCE(ctx, AV_LOG_WARNING, "PPS not present in the stream, nor in AVCC, stream may be unreadable\n");
>                  } else {
> -                    count_or_copy(&out, &out_size, s->pps, s->pps_size, -1, j);
> +                    count_or_copy(&out, &out_size, s->pps, s->pps_size, PS_OUT_OF_BAND, j);
>                  }
>              }
>
> -            count_or_copy(&out, &out_size, buf, nal_size,
> -                          unit_type == H264_NAL_SPS || unit_type == H264_NAL_PPS, j);
> +            if (unit_type == H264_NAL_SPS || unit_type == H264_NAL_PPS)
> +                ps = PS_IN_BAND;
> +            else
> +                ps = PS_NONE;
> +            count_or_copy(&out, &out_size, buf, nal_size, ps, j);
>              if (!new_idr && unit_type == H264_NAL_SLICE) {
>                  new_idr  = 1;
>                  sps_seen = 0;
> --
> 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 a.m. UTC | #3
> On May 29, 2023, at 23:54, mypopy@gmail.com wrote:
> 
> On Fri, May 19, 2023 at 4:41 PM Zhao Zhili <quinkblack@foxmail.com> wrote:
>> 
>> From: Zhao Zhili <zhilizhao@tencent.com>
>> 
>> start_code_size depends on whether PS comes from out-of-band or
>> in-band. Make the code more readable.
>> ---
>> libavcodec/h264_mp4toannexb_bsf.c | 34 ++++++++++++++++++++++++-------
>> 1 file changed, 27 insertions(+), 7 deletions(-)
>> 
>> diff --git a/libavcodec/h264_mp4toannexb_bsf.c b/libavcodec/h264_mp4toannexb_bsf.c
>> index d11be455c2..7dce1ae9b6 100644
>> --- a/libavcodec/h264_mp4toannexb_bsf.c
>> +++ b/libavcodec/h264_mp4toannexb_bsf.c
>> @@ -43,10 +43,26 @@ typedef struct H264BSFContext {
>>     int      extradata_parsed;
>> } H264BSFContext;
>> 
>> +enum PsSource {
>> +    PS_OUT_OF_BAND = -1,
>> +    PS_NONE = 0,
>> +    PS_IN_BAND = 1,
>> +};
>> +
>> static void count_or_copy(uint8_t **out, uint64_t *out_size,
>> -                          const uint8_t *in, int in_size, int ps, int copy)
>> +                          const uint8_t *in, int in_size, enum PsSource ps, int copy)
>> {
>> -    uint8_t start_code_size = ps < 0 ? 0 : *out_size == 0 || ps ? 4 : 3;
>> +    uint8_t start_code_size;
>> +
>> +    if (ps == PS_OUT_OF_BAND)
>> +        /* start code already present in out-of-band ps data, so don't need to
>> +         * add it manually again
>> +         */
>> +        start_code_size = 0;
>> +    else if (ps == PS_IN_BAND || *out_size == 0)
>> +        start_code_size = 4;
>> +    else
>> +        start_code_size = 3;
> I prefer the original style, they are shorter and more concise

It takes a little time to figure out what two ternary do in a
single statement, it takes more time to figure out why, and
multiple ternaries is hard to write comments properly.

Multiple ternaries in a single statement works in simple cases.
For current situation, I prefer ‘if-else’.

>> 
>>     if (copy) {
>>         memcpy(*out + start_code_size, in, in_size);
>> @@ -202,6 +218,7 @@ static int h264_mp4toannexb_filter(AVBSFContext *ctx, AVPacket *opkt)
>> 
>>         do {
>>             uint32_t nal_size = 0;
>> +            enum PsSource ps;
>> 
>>             /* possible overread ok due to padding */
>>             for (int i = 0; i < s->length_size; i++)
>> @@ -230,7 +247,7 @@ static int h264_mp4toannexb_filter(AVBSFContext *ctx, AVPacket *opkt)
>>                     if (!s->sps_size) {
>>                         LOG_ONCE(ctx, AV_LOG_WARNING, "SPS not present in the stream, nor in AVCC, stream may be unreadable\n");
>>                     } else {
>> -                        count_or_copy(&out, &out_size, s->sps, s->sps_size, -1, j);
>> +                        count_or_copy(&out, &out_size, s->sps, s->sps_size, PS_OUT_OF_BAND, j);
>>                         sps_seen = 1;
>>                     }
>>                 }
>> @@ -246,19 +263,22 @@ static int h264_mp4toannexb_filter(AVBSFContext *ctx, AVPacket *opkt)
>>             if (new_idr && unit_type == H264_NAL_IDR_SLICE && !sps_seen && !pps_seen) {
>>                 if (ctx->par_out->extradata)
>>                     count_or_copy(&out, &out_size, ctx->par_out->extradata,
>> -                                  ctx->par_out->extradata_size, -1, j);
>> +                                  ctx->par_out->extradata_size, PS_OUT_OF_BAND, j);
>>                 new_idr = 0;
>>             /* if only SPS has been seen, also insert PPS */
>>             } else if (new_idr && unit_type == H264_NAL_IDR_SLICE && sps_seen && !pps_seen) {
>>                 if (!s->pps_size) {
>>                     LOG_ONCE(ctx, AV_LOG_WARNING, "PPS not present in the stream, nor in AVCC, stream may be unreadable\n");
>>                 } else {
>> -                    count_or_copy(&out, &out_size, s->pps, s->pps_size, -1, j);
>> +                    count_or_copy(&out, &out_size, s->pps, s->pps_size, PS_OUT_OF_BAND, j);
>>                 }
>>             }
>> 
>> -            count_or_copy(&out, &out_size, buf, nal_size,
>> -                          unit_type == H264_NAL_SPS || unit_type == H264_NAL_PPS, j);
>> +            if (unit_type == H264_NAL_SPS || unit_type == H264_NAL_PPS)
>> +                ps = PS_IN_BAND;
>> +            else
>> +                ps = PS_NONE;
>> +            count_or_copy(&out, &out_size, buf, nal_size, ps, j);
>>             if (!new_idr && unit_type == H264_NAL_SLICE) {
>>                 new_idr  = 1;
>>                 sps_seen = 0;
>> --
>> 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".
> 
> 
> 
> -- 
> =======================================
> Jun zhao/赵军
> +++++++++++++++++++++++++++++++++++++++
> _______________________________________________
> 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/libavcodec/h264_mp4toannexb_bsf.c b/libavcodec/h264_mp4toannexb_bsf.c
index d11be455c2..7dce1ae9b6 100644
--- a/libavcodec/h264_mp4toannexb_bsf.c
+++ b/libavcodec/h264_mp4toannexb_bsf.c
@@ -43,10 +43,26 @@  typedef struct H264BSFContext {
     int      extradata_parsed;
 } H264BSFContext;
 
+enum PsSource {
+    PS_OUT_OF_BAND = -1,
+    PS_NONE = 0,
+    PS_IN_BAND = 1,
+};
+
 static void count_or_copy(uint8_t **out, uint64_t *out_size,
-                          const uint8_t *in, int in_size, int ps, int copy)
+                          const uint8_t *in, int in_size, enum PsSource ps, int copy)
 {
-    uint8_t start_code_size = ps < 0 ? 0 : *out_size == 0 || ps ? 4 : 3;
+    uint8_t start_code_size;
+
+    if (ps == PS_OUT_OF_BAND)
+        /* start code already present in out-of-band ps data, so don't need to
+         * add it manually again
+         */
+        start_code_size = 0;
+    else if (ps == PS_IN_BAND || *out_size == 0)
+        start_code_size = 4;
+    else
+        start_code_size = 3;
 
     if (copy) {
         memcpy(*out + start_code_size, in, in_size);
@@ -202,6 +218,7 @@  static int h264_mp4toannexb_filter(AVBSFContext *ctx, AVPacket *opkt)
 
         do {
             uint32_t nal_size = 0;
+            enum PsSource ps;
 
             /* possible overread ok due to padding */
             for (int i = 0; i < s->length_size; i++)
@@ -230,7 +247,7 @@  static int h264_mp4toannexb_filter(AVBSFContext *ctx, AVPacket *opkt)
                     if (!s->sps_size) {
                         LOG_ONCE(ctx, AV_LOG_WARNING, "SPS not present in the stream, nor in AVCC, stream may be unreadable\n");
                     } else {
-                        count_or_copy(&out, &out_size, s->sps, s->sps_size, -1, j);
+                        count_or_copy(&out, &out_size, s->sps, s->sps_size, PS_OUT_OF_BAND, j);
                         sps_seen = 1;
                     }
                 }
@@ -246,19 +263,22 @@  static int h264_mp4toannexb_filter(AVBSFContext *ctx, AVPacket *opkt)
             if (new_idr && unit_type == H264_NAL_IDR_SLICE && !sps_seen && !pps_seen) {
                 if (ctx->par_out->extradata)
                     count_or_copy(&out, &out_size, ctx->par_out->extradata,
-                                  ctx->par_out->extradata_size, -1, j);
+                                  ctx->par_out->extradata_size, PS_OUT_OF_BAND, j);
                 new_idr = 0;
             /* if only SPS has been seen, also insert PPS */
             } else if (new_idr && unit_type == H264_NAL_IDR_SLICE && sps_seen && !pps_seen) {
                 if (!s->pps_size) {
                     LOG_ONCE(ctx, AV_LOG_WARNING, "PPS not present in the stream, nor in AVCC, stream may be unreadable\n");
                 } else {
-                    count_or_copy(&out, &out_size, s->pps, s->pps_size, -1, j);
+                    count_or_copy(&out, &out_size, s->pps, s->pps_size, PS_OUT_OF_BAND, j);
                 }
             }
 
-            count_or_copy(&out, &out_size, buf, nal_size,
-                          unit_type == H264_NAL_SPS || unit_type == H264_NAL_PPS, j);
+            if (unit_type == H264_NAL_SPS || unit_type == H264_NAL_PPS)
+                ps = PS_IN_BAND;
+            else
+                ps = PS_NONE;
+            count_or_copy(&out, &out_size, buf, nal_size, ps, j);
             if (!new_idr && unit_type == H264_NAL_SLICE) {
                 new_idr  = 1;
                 sps_seen = 0;