Re: [FFmpeg-devel] [PATCH v11 1/6] libavutil/wchar_filename.h: Add whcartoutf8, wchartoansi and utf8toansi

[Soft Works <softworkz@hotmail.com>]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of nil-
> admirari@mailo.com
> Sent: Sunday, April 24, 2022 2:08 PM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v11 1/6]
> libavutil/wchar_filename.h: Add whcartoutf8, wchartoansi and
> utf8toansi
> 
> > 1. Patch 3/6 - Replace LoadLibraryExA with LoadLibraryExW
> > What's the point in making changes to library loading? What does it
> fix?
> 
> 
> 
> From the commit https://ffmpeg.org/pipermail/ffmpeg-devel/2022-
> April/295571.html:
> 
> ... the path length limitation is only lifted for file APIs that pass
> paths as wchar_t...

You normally don't load libraries from such longs paths. As I had written 
previously (but stripped off by you):

Could you please give an example of a situation that this is supposed 
to fix?

Which dll do you think would need to be loaded from a long path?
----------------------------------------------------------------


> > 2. Patches 5/6 and 6/6 - Add Fusion Manifest
> > ...
> > Both of these manifest attributes are affecting the runtime behavior
> of
> > an application running on Windows - but only starting from a certain
> > OS version.
> > ... you would need to check the operating system version
> > before using to make sure that you are providing parameters in the
> "right"
> > way - I'm not sure whether that would make much sense.
> 
> 
> The same can be said about e.g. DirectX 11 hardware acceleration,
> which is available
> only starting with Windows 7; and you have to check OS version before
> providing
> parameters that enable it. Does not mean implementing that feature
> made no sense,
> since it made FFmpeg inconsistent with previous versions of Windows.
> Consistency
> 
> with the greatest common denominator does not make much sense.

Your example is making a comparison that is not quite valid IMO.
The use of DirectX 11 is a very specific feature and that surely 
involves checking of availability.

But file IO is a fundamental feature where a common and predictable 
behavior is crucial.

----------------------------------------------------------------
 
> > 3. All Patches x/6 - Remove MAX_PATH limit
> > The punch line sounds compelling. But how is the current situation
> and
> > what exactly would be the benefit?
> 
> All patches with that line replace MAX_PATH-sized buffers with
> dynamically-allocated
> 
> buffers of potentially arbitrary size. If application continues to use
> fixed-size buffers,
> 
> it's not long path aware, regardless of the manifest.

That's true, but the file IO in ffmpeg does not use MAX_PATH buffers,
so it's only about those few specific cases:

Patch 2/6: Removing the fixed-size buffer is surely a good change,
           but can't you just completely remove the charset 
           conversion and use the same file IO patterns that ffmpeg
           is using everywhere else?
Patch 3/6: see 1.
Patch 4/6: Seems to be pointless because you cannot run ffmpeg.exe from
           a long path anyway. Neither with cmd nor with powershell.
           Even if you could, it would be a pretty exotic use case.


> It is neither predictable nor reliable. Please check
> https://googleprojectzero.blogspot.com/2016/02/the-definitive-guide-
> on-win32-to-nt.html
> 
> for things like “I actually exploited this behaviour to create
> arbitrary named pipes from the Chrome sandbox awhile back.”

I know about that. But it's something that is possible now already
and would be still possible after applying your patchset.
So, that's not a relevant point in this regard.


> > 1. Windows version needs to be >= 1903
> > (for being able to have both attributes in effect)
> For all intents and purposes, versions of Windows < 1903 have EOLed.
> Exceptions:
> 
> - Windows 8.1 will EOL in January 2023,
> - LTSB/LTSC branches of Windows 10 before 21H2.

And the servers? And all those which are still using older 
versions?`

> > 3. A registry key or group policy needs to be set on Windows to
> enable this
> > ´ (in both cases, administrative permission/UAC is required to set
> it)
> > 4. Even when registry key or group policy is set, it might still be
> pending
> > a reboot
> 
> Is it a big deal to change a registry and reboot?

Yes it is. Especially when you don't know that it's needed or whether
it's needed or when it is needed.
Also it requires administrative permissions which not everybody has.
Further it's a serious change to OS behavior and you cannot expect that 
all users are educated enough for being able to make this decision 
and be confident that it won't have any unexpected side effects.

> > On the other side, there's a risk of regressions by adding those
> manifest
> > attributes.
> 
> Adding anything risks regressions, how manifest is supposed to be
> different from any other patch?

That's what I tried to explain. "Any other patch" makes a change
after which you know that the change has been made.

But adding those attributes will impose changes that will be effective
under certain conditions only and about which neither the user nor the
application "knows" whether they are effective or not.

> > I wonder whether it wouldn’t be a better idea, to simply auto-add
> this prefix
> > in the ffmpeg file handling code if:
> 
> Every command line argument that supplies a path needs to be updated.
> Every filter with a file argument needs to be updated.

No. There's are common file APIs where such change could be made.

Best regards,
softworkz

PS: Your line breaks were all doubled. Please check before replying.


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