diff mbox series

[FFmpeg-devel] avformat/mov: skip call ff_codec_get_id if possible

Message ID tencent_4134E33FA556E99D2EE0705A383403803809@qq.com
State Accepted
Commit 580fb6a8c94e7a41c8160186289fd852a9c1f5cd
Headers show
Series [FFmpeg-devel] avformat/mov: skip call ff_codec_get_id if 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
andriy/make_armv7_RPi4 success Make finished
andriy/make_fate_armv7_RPi4 success Make fate finished

Commit Message

Zhao Zhili April 19, 2022, 5:12 a.m. UTC 
ff_codec_get_id loops over ff_codec_movvideo_tags (which is a large
array) two times. The result is unused most of the cases.
---
 libavformat/mov.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Comments

Zhao Zhili April 29, 2022, 9:46 a.m. UTC | #1 
> On Apr 19, 2022, at 1:12 PM, Zhao Zhili <quinkblack@foxmail.com> wrote:
> 
> ff_codec_get_id loops over ff_codec_movvideo_tags (which is a large
> array) two times. The result is unused most of the cases.
> ---
> libavformat/mov.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index 6c847de164..bdc8c84bae 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -2487,8 +2487,6 @@ static int mov_skip_multiple_stsd(MOVContext *c, AVIOContext *pb,
>                                   int codec_tag, int format,
>                                   int64_t size)
> {
> -    int video_codec_id = ff_codec_get_id(ff_codec_movvideo_tags, format);
> -
>     if (codec_tag &&
>          (codec_tag != format &&
>           // AVID 1:1 samples with differing data format and codec tag exist
> @@ -2497,7 +2495,7 @@ static int mov_skip_multiple_stsd(MOVContext *c, AVIOContext *pb,
>           codec_tag != AV_RL32("apcn") && codec_tag != AV_RL32("apch") &&
>           // so is dv (sigh)
>           codec_tag != AV_RL32("dvpp") && codec_tag != AV_RL32("dvcp") &&
> -          (c->fc->video_codec_id ? video_codec_id != c->fc->video_codec_id
> +          (c->fc->video_codec_id ? ff_codec_get_id(ff_codec_movvideo_tags, format) != c->fc->video_codec_id
>                                  : codec_tag != MKTAG('j','p','e','g')))) {
>         /* Multiple fourcc, we skip JPEG. This is not correct, we should
>          * export it as a separate AVStream but this needs a few changes
> -- 
> 2.35.3
> 

Do we care about such micro-optimization, or just let compiler do it’s job?
Andreas Rheinhardt April 29, 2022, 11:26 a.m. UTC | #2 
"zhilizhao(赵志立)":
> 
> 
>> On Apr 19, 2022, at 1:12 PM, Zhao Zhili <quinkblack@foxmail.com> wrote:
>>
>> ff_codec_get_id loops over ff_codec_movvideo_tags (which is a large
>> array) two times. The result is unused most of the cases.
>> ---
>> libavformat/mov.c | 4 +---
>> 1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>> index 6c847de164..bdc8c84bae 100644
>> --- a/libavformat/mov.c
>> +++ b/libavformat/mov.c
>> @@ -2487,8 +2487,6 @@ static int mov_skip_multiple_stsd(MOVContext *c, AVIOContext *pb,
>>                                   int codec_tag, int format,
>>                                   int64_t size)
>> {
>> -    int video_codec_id = ff_codec_get_id(ff_codec_movvideo_tags, format);
>> -
>>     if (codec_tag &&
>>          (codec_tag != format &&
>>           // AVID 1:1 samples with differing data format and codec tag exist
>> @@ -2497,7 +2495,7 @@ static int mov_skip_multiple_stsd(MOVContext *c, AVIOContext *pb,
>>           codec_tag != AV_RL32("apcn") && codec_tag != AV_RL32("apch") &&
>>           // so is dv (sigh)
>>           codec_tag != AV_RL32("dvpp") && codec_tag != AV_RL32("dvcp") &&
>> -          (c->fc->video_codec_id ? video_codec_id != c->fc->video_codec_id
>> +          (c->fc->video_codec_id ? ff_codec_get_id(ff_codec_movvideo_tags, format) != c->fc->video_codec_id
>>                                  : codec_tag != MKTAG('j','p','e','g')))) {
>>         /* Multiple fourcc, we skip JPEG. This is not correct, we should
>>          * export it as a separate AVStream but this needs a few changes
>> -- 
>> 2.35.3
>>
> 
> Do we care about such micro-optimization, or just let compiler do it’s job?

A compiler could currently only optimize this on its own if you were
using LTO; it might also be able to optimize this if ff_codec_get_id
were marked as av_pure. Of course, there is no guarantee that this is
optimized in these cases, but you could check whether e.g. GCC/Clang do it.
And I don't see anything wrong with micro-optimizations.

- Andreas
diff mbox series

Patch

diff --git a/libavformat/mov.c b/libavformat/mov.c
index 6c847de164..bdc8c84bae 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -2487,8 +2487,6 @@  static int mov_skip_multiple_stsd(MOVContext *c, AVIOContext *pb,
                                   int codec_tag, int format,
                                   int64_t size)
 {
-    int video_codec_id = ff_codec_get_id(ff_codec_movvideo_tags, format);
-
     if (codec_tag &&
          (codec_tag != format &&
           // AVID 1:1 samples with differing data format and codec tag exist
@@ -2497,7 +2495,7 @@  static int mov_skip_multiple_stsd(MOVContext *c, AVIOContext *pb,
           codec_tag != AV_RL32("apcn") && codec_tag != AV_RL32("apch") &&
           // so is dv (sigh)
           codec_tag != AV_RL32("dvpp") && codec_tag != AV_RL32("dvcp") &&
-          (c->fc->video_codec_id ? video_codec_id != c->fc->video_codec_id
+          (c->fc->video_codec_id ? ff_codec_get_id(ff_codec_movvideo_tags, format) != c->fc->video_codec_id
                                  : codec_tag != MKTAG('j','p','e','g')))) {
         /* Multiple fourcc, we skip JPEG. This is not correct, we should
          * export it as a separate AVStream but this needs a few changes