Re: [FFmpeg-devel] [PATCH 1/3] fftools: Stop using av_fopen_utf8

From: "Martin Storsjö" <martin@martin.st>
To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Subject: Re: [FFmpeg-devel] [PATCH 1/3] fftools: Stop using av_fopen_utf8
Date: Tue, 24 May 2022 23:21:55 +0300 (EEST)
Message-ID: <d697696b-8ad1-f5c7-ffcc-1d811d494a56@martin.st> (raw)
In-Reply-To: <DM8P223MB03659671D3D9C3B4AB8F590FBAD79@DM8P223MB0365.NAMP223.PROD.OUTLOOK.COM>

On Tue, 24 May 2022, Soft Works wrote:

>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Martin
>> Storsjö
>> Sent: Tuesday, May 24, 2022 11:29 AM
>> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
>> Subject: Re: [FFmpeg-devel] [PATCH 1/3] fftools: Stop using av_fopen_utf8
>>
>> On Mon, 23 May 2022, Soft Works wrote:
>>
>>> Great. I rebased and resubmitted both patchsets. The primary long-path
>>> patchset didn't need any change.
>>>
>>> Considerations for the latter were:
>>>
>>> - Should the file wchar_filename.h be renamed as it is now containing
>>>  the path prefixing code?
>>
>> I guess we could do that later at some point, but I don't see it as
>> urgent.
>>
>>> - I have kept the path functions in the same way like .NET does it,
>>>  just for easy reference and following. Compilers will inline
>>>  them anyway (my pov). Maybe others don't like that. I can change
>>>  if it's got to be.
>>
>> Having the individual functions inline compared to merging it all in one
>> big function doesn't matter to me. But the amount of code in this inline
>> header is growing a bit big, to the point that I think it is measurable
>> when multiple files within the same library use these functions. Longer
>> term, it would probably make sense to make them more shared in some way.
>
>> If C would have the C++ style deduplication feature for non-static inline
>> functions, this wouldn't be an issue. One could consider making them ff_
>> functions and carry a copy in each library too, maybe. (But that also
>> makes it trickier to use in fftools.)
>
> A copy in each library - isn't that exactly what's already happening?
>
> The get_extended_win32_path() is used from 2 places:
>
> 2. os_support.h
>
> This is used in libavformat exclusively. But in this case, the code gets
> duplicated actually for each consumption of one of those file functions.
> There aren't many usages in total, though, and I don't see any trend
> on the horizon for sudden increase, so I agree that there's no urgency.

Yes, this is what I was referring to. It's clearly more than one use. When 
counting files that use mkdir, unlink or "struct stat", I find around 9 
individual .c files in libavformat, that would end up with static inline 
copies of all of this.

Compared with the av_fopen_utf8 case, I first tested fixing it by making 
it a static inline function in a libavutil header, but that did increase 
the size of a built DLL by a couple KB. I guess this increases it by a 
bit more than that.

It's still not a lot, and this isn't a blocker (and I probably prefer that 
we don't try to redesign this issue here now, in order not to drag out the 
review further), but compared to how C++ inline methods are deduplicated 
by the linker, it's a slightly annoying inefficiency.

// Martin
_______________________________________________
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".