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

[Martin Storsjö]

On Tue, 24 May 2022, Soft Works wrote:

>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of Martin
>> Storsjö
>> Sent: Tuesday, May 24, 2022 10:22 PM
>> To: FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org>
>> Subject: Re: [FFmpeg-devel] [PATCH 1/3] fftools: Stop using av_fopen_utf8
>>
>> On Tue, 24 May 2022, Soft Works wrote:
>>
>>>> -----Original Message-----
>>>> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
>> Martin
>>>> Storsjö
>>>> Sent: Tuesday, May 24, 2022 11:29 AM
>>>> To: FFmpeg development discussions and patches <ffmpeg-
>> devel at 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)
>
> I'm glad you're saying that ;-)
>
> It probably won't be difficult to improve this in a future change, making it
> a wchar_filename.c plus wchar_filename.h file in libavutil. If I'm not
> mistaken, there shouldn't be an issue with multiple CRTs as memory is
> managed by av_ functions?

Indeed, that's not a problem for plain filename conversions. The main 
hesitation is that we'd generally want to avoid adding more avpriv_ APIs 
unless strictly necessary.

// Martin