diff mbox series

[FFmpeg-devel,v3,7/8] avcodec/movtextenc: Remove redundant function parameters

Message ID 20201017073745.403153-7-andreas.rheinhardt@gmail.com
State Accepted
Commit eab812d6d61912cddbfbe72ad4419286eb43514d
Headers show
Series [FFmpeg-devel,v3,1/8] avcodec/movtextenc: Fix potential use of uninitialized value
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished

Commit Message

Andreas Rheinhardt Oct. 17, 2020, 7:37 a.m. UTC
It makes no sense to call the functions to write styl, hlit or hclr boxes
with a different box name than "styl", "hlit" or "hclr". Therefore this
commit inlines these values in the functions, removes the function
parameter containing the box's name and removes the (non obsolete) box
names from the list of boxes.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
 libavcodec/movtextenc.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

Comments

Zhao Zhili Oct. 17, 2020, 4:10 p.m. UTC | #1
> On Oct 17, 2020, at 3:37 PM, Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote:
> 
> It makes no sense to call the functions to write styl, hlit or hclr boxes
> with a different box name than "styl", "hlit" or "hclr". Therefore this
> commit inlines these values in the functions, removes the function
> parameter containing the box's name and removes the (non obsolete) box
> names from the list of boxes.

The extra parameter is useless for now, but it may add some flexibility in the
future, for example, two box types with similar handle logic. I don't know
whether such thing can happen.

> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
> libavcodec/movtextenc.c | 23 +++++++++++------------
> 1 file changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/libavcodec/movtextenc.c b/libavcodec/movtextenc.c
> index 908b2bfde5..2082dc9b25 100644
> --- a/libavcodec/movtextenc.c
> +++ b/libavcodec/movtextenc.c
> @@ -92,8 +92,7 @@ typedef struct {
> } MovTextContext;
> 
> typedef struct {
> -    uint32_t type;
> -    void (*encode)(MovTextContext *s, uint32_t tsmb_type);
> +    void (*encode)(MovTextContext *s);
> } Box;
> 
> static void mov_text_cleanup(MovTextContext *s)
> @@ -102,13 +101,13 @@ static void mov_text_cleanup(MovTextContext *s)
>     s->style_attributes_temp = s->d;
> }
> 
> -static void encode_styl(MovTextContext *s, uint32_t tsmb_type)
> +static void encode_styl(MovTextContext *s)
> {
>     if ((s->box_flags & STYL_BOX) && s->count) {
>         uint8_t buf[12], *p = buf;
> 
>         bytestream_put_be32(&p, s->count * STYLE_RECORD_SIZE + SIZE_ADD);
> -        bytestream_put_be32(&p, tsmb_type);
> +        bytestream_put_be32(&p, MKBETAG('s','t','y','l'));
>         bytestream_put_be16(&p, s->count);
>         /*The above three attributes are hard coded for now
>         but will come from ASS style in the future*/
> @@ -130,13 +129,13 @@ static void encode_styl(MovTextContext *s, uint32_t tsmb_type)
>     mov_text_cleanup(s);
> }
> 
> -static void encode_hlit(MovTextContext *s, uint32_t tsmb_type)
> +static void encode_hlit(MovTextContext *s)
> {
>     if (s->box_flags & HLIT_BOX) {
>         uint8_t buf[12], *p = buf;
> 
>         bytestream_put_be32(&p, 12);
> -        bytestream_put_be32(&p, tsmb_type);
> +        bytestream_put_be32(&p, MKBETAG('h','l','i','t'));
>         bytestream_put_be16(&p, s->hlit.start);
>         bytestream_put_be16(&p, s->hlit.end);
> 
> @@ -144,13 +143,13 @@ static void encode_hlit(MovTextContext *s, uint32_t tsmb_type)
>     }
> }
> 
> -static void encode_hclr(MovTextContext *s, uint32_t tsmb_type)
> +static void encode_hclr(MovTextContext *s)
> {
>     if (s->box_flags & HCLR_BOX) {
>         uint8_t buf[12], *p = buf;
> 
>         bytestream_put_be32(&p, 12);
> -        bytestream_put_be32(&p, tsmb_type);
> +        bytestream_put_be32(&p, MKBETAG('h','c','l','r'));
>         bytestream_put_be32(&p, s->hclr.color);
> 
>         av_bprint_append_any(&s->buffer, buf, 12);
> @@ -158,9 +157,9 @@ static void encode_hclr(MovTextContext *s, uint32_t tsmb_type)
> }
> 
> static const Box box_types[] = {
> -    { MKBETAG('s','t','y','l'), encode_styl },
> -    { MKBETAG('h','l','i','t'), encode_hlit },
> -    { MKBETAG('h','c','l','r'), encode_hclr },
> +    { encode_styl },
> +    { encode_hlit },
> +    { encode_hclr },
> };
> 
> const static size_t box_count = FF_ARRAY_ELEMS(box_types);
> @@ -682,7 +681,7 @@ static int mov_text_encode_frame(AVCodecContext *avctx, unsigned char *buf,
> #endif
> 
>         for (j = 0; j < box_count; j++) {
> -            box_types[j].encode(s, box_types[j].type);
> +            box_types[j].encode(s);
>         }
>     }
> 
> -- 
> 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".
Andreas Rheinhardt Oct. 17, 2020, 4:14 p.m. UTC | #2
Zhao Zhili:
> 
> 
>> On Oct 17, 2020, at 3:37 PM, Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote:
>>
>> It makes no sense to call the functions to write styl, hlit or hclr boxes
>> with a different box name than "styl", "hlit" or "hclr". Therefore this
>> commit inlines these values in the functions, removes the function
>> parameter containing the box's name and removes the (non obsolete) box
>> names from the list of boxes.
> 
> The extra parameter is useless for now, but it may add some flexibility in the
> future, for example, two box types with similar handle logic. I don't know
> whether such thing can happen.
> 

Of course the parameter could be readded in the future if there is a
usecase for it (after all, there are no ABI/API constraints as this is
all internal to this translation unit). But right now this parameter has
no benefit whatsoever.

>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>> ---
>> libavcodec/movtextenc.c | 23 +++++++++++------------
>> 1 file changed, 11 insertions(+), 12 deletions(-)
>>
>> diff --git a/libavcodec/movtextenc.c b/libavcodec/movtextenc.c
>> index 908b2bfde5..2082dc9b25 100644
>> --- a/libavcodec/movtextenc.c
>> +++ b/libavcodec/movtextenc.c
>> @@ -92,8 +92,7 @@ typedef struct {
>> } MovTextContext;
>>
>> typedef struct {
>> -    uint32_t type;
>> -    void (*encode)(MovTextContext *s, uint32_t tsmb_type);
>> +    void (*encode)(MovTextContext *s);
>> } Box;
>>
>> static void mov_text_cleanup(MovTextContext *s)
>> @@ -102,13 +101,13 @@ static void mov_text_cleanup(MovTextContext *s)
>>     s->style_attributes_temp = s->d;
>> }
>>
>> -static void encode_styl(MovTextContext *s, uint32_t tsmb_type)
>> +static void encode_styl(MovTextContext *s)
>> {
>>     if ((s->box_flags & STYL_BOX) && s->count) {
>>         uint8_t buf[12], *p = buf;
>>
>>         bytestream_put_be32(&p, s->count * STYLE_RECORD_SIZE + SIZE_ADD);
>> -        bytestream_put_be32(&p, tsmb_type);
>> +        bytestream_put_be32(&p, MKBETAG('s','t','y','l'));
>>         bytestream_put_be16(&p, s->count);
>>         /*The above three attributes are hard coded for now
>>         but will come from ASS style in the future*/
>> @@ -130,13 +129,13 @@ static void encode_styl(MovTextContext *s, uint32_t tsmb_type)
>>     mov_text_cleanup(s);
>> }
>>
>> -static void encode_hlit(MovTextContext *s, uint32_t tsmb_type)
>> +static void encode_hlit(MovTextContext *s)
>> {
>>     if (s->box_flags & HLIT_BOX) {
>>         uint8_t buf[12], *p = buf;
>>
>>         bytestream_put_be32(&p, 12);
>> -        bytestream_put_be32(&p, tsmb_type);
>> +        bytestream_put_be32(&p, MKBETAG('h','l','i','t'));
>>         bytestream_put_be16(&p, s->hlit.start);
>>         bytestream_put_be16(&p, s->hlit.end);
>>
>> @@ -144,13 +143,13 @@ static void encode_hlit(MovTextContext *s, uint32_t tsmb_type)
>>     }
>> }
>>
>> -static void encode_hclr(MovTextContext *s, uint32_t tsmb_type)
>> +static void encode_hclr(MovTextContext *s)
>> {
>>     if (s->box_flags & HCLR_BOX) {
>>         uint8_t buf[12], *p = buf;
>>
>>         bytestream_put_be32(&p, 12);
>> -        bytestream_put_be32(&p, tsmb_type);
>> +        bytestream_put_be32(&p, MKBETAG('h','c','l','r'));
>>         bytestream_put_be32(&p, s->hclr.color);
>>
>>         av_bprint_append_any(&s->buffer, buf, 12);
>> @@ -158,9 +157,9 @@ static void encode_hclr(MovTextContext *s, uint32_t tsmb_type)
>> }
>>
>> static const Box box_types[] = {
>> -    { MKBETAG('s','t','y','l'), encode_styl },
>> -    { MKBETAG('h','l','i','t'), encode_hlit },
>> -    { MKBETAG('h','c','l','r'), encode_hclr },
>> +    { encode_styl },
>> +    { encode_hlit },
>> +    { encode_hclr },
>> };
>>
>> const static size_t box_count = FF_ARRAY_ELEMS(box_types);
>> @@ -682,7 +681,7 @@ static int mov_text_encode_frame(AVCodecContext *avctx, unsigned char *buf,
>> #endif
>>
>>         for (j = 0; j < box_count; j++) {
>> -            box_types[j].encode(s, box_types[j].type);
>> +            box_types[j].encode(s);
>>         }
>>     }
>>
>> -- 
>> 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".
>
diff mbox series

Patch

diff --git a/libavcodec/movtextenc.c b/libavcodec/movtextenc.c
index 908b2bfde5..2082dc9b25 100644
--- a/libavcodec/movtextenc.c
+++ b/libavcodec/movtextenc.c
@@ -92,8 +92,7 @@  typedef struct {
 } MovTextContext;
 
 typedef struct {
-    uint32_t type;
-    void (*encode)(MovTextContext *s, uint32_t tsmb_type);
+    void (*encode)(MovTextContext *s);
 } Box;
 
 static void mov_text_cleanup(MovTextContext *s)
@@ -102,13 +101,13 @@  static void mov_text_cleanup(MovTextContext *s)
     s->style_attributes_temp = s->d;
 }
 
-static void encode_styl(MovTextContext *s, uint32_t tsmb_type)
+static void encode_styl(MovTextContext *s)
 {
     if ((s->box_flags & STYL_BOX) && s->count) {
         uint8_t buf[12], *p = buf;
 
         bytestream_put_be32(&p, s->count * STYLE_RECORD_SIZE + SIZE_ADD);
-        bytestream_put_be32(&p, tsmb_type);
+        bytestream_put_be32(&p, MKBETAG('s','t','y','l'));
         bytestream_put_be16(&p, s->count);
         /*The above three attributes are hard coded for now
         but will come from ASS style in the future*/
@@ -130,13 +129,13 @@  static void encode_styl(MovTextContext *s, uint32_t tsmb_type)
     mov_text_cleanup(s);
 }
 
-static void encode_hlit(MovTextContext *s, uint32_t tsmb_type)
+static void encode_hlit(MovTextContext *s)
 {
     if (s->box_flags & HLIT_BOX) {
         uint8_t buf[12], *p = buf;
 
         bytestream_put_be32(&p, 12);
-        bytestream_put_be32(&p, tsmb_type);
+        bytestream_put_be32(&p, MKBETAG('h','l','i','t'));
         bytestream_put_be16(&p, s->hlit.start);
         bytestream_put_be16(&p, s->hlit.end);
 
@@ -144,13 +143,13 @@  static void encode_hlit(MovTextContext *s, uint32_t tsmb_type)
     }
 }
 
-static void encode_hclr(MovTextContext *s, uint32_t tsmb_type)
+static void encode_hclr(MovTextContext *s)
 {
     if (s->box_flags & HCLR_BOX) {
         uint8_t buf[12], *p = buf;
 
         bytestream_put_be32(&p, 12);
-        bytestream_put_be32(&p, tsmb_type);
+        bytestream_put_be32(&p, MKBETAG('h','c','l','r'));
         bytestream_put_be32(&p, s->hclr.color);
 
         av_bprint_append_any(&s->buffer, buf, 12);
@@ -158,9 +157,9 @@  static void encode_hclr(MovTextContext *s, uint32_t tsmb_type)
 }
 
 static const Box box_types[] = {
-    { MKBETAG('s','t','y','l'), encode_styl },
-    { MKBETAG('h','l','i','t'), encode_hlit },
-    { MKBETAG('h','c','l','r'), encode_hclr },
+    { encode_styl },
+    { encode_hlit },
+    { encode_hclr },
 };
 
 const static size_t box_count = FF_ARRAY_ELEMS(box_types);
@@ -682,7 +681,7 @@  static int mov_text_encode_frame(AVCodecContext *avctx, unsigned char *buf,
 #endif
 
         for (j = 0; j < box_count; j++) {
-            box_types[j].encode(s, box_types[j].type);
+            box_types[j].encode(s);
         }
     }