diff mbox series

[FFmpeg-devel,5/7] avcodec/vc2enc: Avoid relocations for short strings

Message ID AS8P250MB0744309AA8FCF83F7753405E8FF12@AS8P250MB0744.EURP250.PROD.OUTLOOK.COM
State New
Headers show
Series [FFmpeg-devel,1/7] avcodec/vc2enc: Avoid void* where possible | 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

Andreas Rheinhardt May 28, 2024, 2:49 a.m. UTC
These strings are so short that they can be put directly
into the containing structure, avoiding the pointer
and putting it into .rodata.
Also use chars for interlaced and level while at it, as
these are so small.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/vc2enc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

James Almer May 28, 2024, 3:07 a.m. UTC | #1
On 5/27/2024 11:49 PM, Andreas Rheinhardt wrote:
> These strings are so short that they can be put directly
> into the containing structure, avoiding the pointer
> and putting it into .rodata.
> Also use chars for interlaced and level while at it, as
> these are so small.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>   libavcodec/vc2enc.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/vc2enc.c b/libavcodec/vc2enc.c
> index 3285218724..8b9641916a 100644
> --- a/libavcodec/vc2enc.c
> +++ b/libavcodec/vc2enc.c
> @@ -41,8 +41,9 @@
>   typedef struct VC2BaseVideoFormat {
>       enum AVPixelFormat pix_fmt;
>       AVRational time_base;
> -    int width, height, interlaced, level;
> -    const char *name;
> +    int width, height;
> +    char interlaced, level;

Use a fixed size type like uint8_t and not char. Neither of these values 
are characters (interlace should strictly speaking be a bool, but afaict 
that's not portable).

> +    char name[13];
>   } VC2BaseVideoFormat;
>   
>   static const VC2BaseVideoFormat base_video_fmts[] = {
Rémi Denis-Courmont May 28, 2024, 6:38 a.m. UTC | #2
Le 28 mai 2024 06:07:41 GMT+03:00, James Almer <jamrial@gmail.com> a écrit :
>On 5/27/2024 11:49 PM, Andreas Rheinhardt wrote:
>> These strings are so short that they can be put directly
>> into the containing structure, avoiding the pointer
>> and putting it into .rodata.
>> Also use chars for interlaced and level while at it, as
>> these are so small.
>> 
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>> ---
>>   libavcodec/vc2enc.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>> 
>> diff --git a/libavcodec/vc2enc.c b/libavcodec/vc2enc.c
>> index 3285218724..8b9641916a 100644
>> --- a/libavcodec/vc2enc.c
>> +++ b/libavcodec/vc2enc.c
>> @@ -41,8 +41,9 @@
>>   typedef struct VC2BaseVideoFormat {
>>       enum AVPixelFormat pix_fmt;
>>       AVRational time_base;
>> -    int width, height, interlaced, level;
>> -    const char *name;
>> +    int width, height;
>> +    char interlaced, level;
>
>Use a fixed size type like uint8_t and not char.

The size of `char` is fixed to 8 bits *if* `uint8_t` exists though. It may be preferable to pick an explicitly signed or unsigned type still.
Lynne May 28, 2024, 2:25 p.m. UTC | #3
On 28/05/2024 04:49, Andreas Rheinhardt wrote:
> These strings are so short that they can be put directly
> into the containing structure, avoiding the pointer
> and putting it into .rodata.
> Also use chars for interlaced and level while at it, as
> these are so small.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>   libavcodec/vc2enc.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/vc2enc.c b/libavcodec/vc2enc.c
> index 3285218724..8b9641916a 100644
> --- a/libavcodec/vc2enc.c
> +++ b/libavcodec/vc2enc.c
> @@ -41,8 +41,9 @@
>   typedef struct VC2BaseVideoFormat {
>       enum AVPixelFormat pix_fmt;
>       AVRational time_base;
> -    int width, height, interlaced, level;
> -    const char *name;
> +    int width, height;
> +    char interlaced, level;
> +    char name[13];
>   } VC2BaseVideoFormat;
>   
>   static const VC2BaseVideoFormat base_video_fmts[] = {

Patchset LGTM if interlaced and level are uint8_t like other commented.
diff mbox series

Patch

diff --git a/libavcodec/vc2enc.c b/libavcodec/vc2enc.c
index 3285218724..8b9641916a 100644
--- a/libavcodec/vc2enc.c
+++ b/libavcodec/vc2enc.c
@@ -41,8 +41,9 @@ 
 typedef struct VC2BaseVideoFormat {
     enum AVPixelFormat pix_fmt;
     AVRational time_base;
-    int width, height, interlaced, level;
-    const char *name;
+    int width, height;
+    char interlaced, level;
+    char name[13];
 } VC2BaseVideoFormat;
 
 static const VC2BaseVideoFormat base_video_fmts[] = {