1425219 - Stop using GetNativePath in ffvpx

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P3)

Comment 1

[Masatoshi Kimura [:emk]]

Attached a patch in bug 685236.

Comment 2

[Masatoshi Kimura [:emk]]

I'll move ffvpx part from bug 685236 to this bug.

Comment 3

[Masatoshi Kimura [:emk]]

(In reply to Jean-Yves Avenard [:jya] from bug 685236 comment #50)
> there's no commit message.
> 
> Additionally, the windows replacement for PR_GetLibraryName shouldn't be
> found in this file, it's totally out of scope.
> 
> Please, create a generic PR_GetLibraryName that will work on windows as it
> should rather than having multiple implementations wherever it's used

PR_GetLibraryName can't be generic because NSPR is written in C (not C++).
Does it make sense to add NS_GetLibraryName to XPCOM?

Comment 4

[Masatoshi Kimura [:emk]]

Attachment: Bug 1425219 - Stop using GetNativePath in ffvpx.

FFVPXRuntimeLinker could not handle file paths that contain characters outside the current system code page on Windows. This patch will fix it by using wide char APIs.

Review commit: https://reviewboard.mozilla.org/r/220158/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/220158/

Comment 5

[Jean-Yves Avenard [:jya]]

Comment on attachment 8950899 [details]
Bug 1425219 - Stop using GetNativePath in ffvpx.

https://reviewboard.mozilla.org/r/220158/#review227056

PathString needs to be better abstracted so that its use is not platform dependent

::: dom/media/platforms/ffmpeg/ffvpx/FFVPXRuntimeLinker.cpp:53
(Diff revision 1)
>  #else
>    PRLibrary* lib = PR_LoadLibraryWithFlags(lspec, PR_LD_NOW | PR_LD_LOCAL);
>  #endif
>    if (!lib) {
> -    FFMPEG_LOG("unable to load library %s", aName);
> +#ifdef XP_WIN
> +    FFMPEG_LOG("unable to load library %s", NS_ConvertUTF16toUTF8(aName).get());

it's rather disappointing that an abstract class like PathString could return a different argument depending on the platform it's running on:
e.g. utf8 or utf16

Why couldn't that be abstracted too?

Comment 6

[Masatoshi Kimura [:emk]]

Comment on attachment 8950899 [details]
Bug 1425219 - Stop using GetNativePath in ffvpx.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220158/diff/1-2/

Comment 7

[Masatoshi Kimura [:emk]]

Comment on attachment 8950899 [details]
Bug 1425219 - Stop using GetNativePath in ffvpx.

https://reviewboard.mozilla.org/r/220158/#review227056

> it's rather disappointing that an abstract class like PathString could return a different argument depending on the platform it's running on:
> e.g. utf8 or utf16
> 
> Why couldn't that be abstracted too?

PathString is based on the C++17 std::filesystem::path::string_type that depends on platforms. The purpose of PathString is not such abstraction. Even if we use the same character type, PathString will depend on platforms in some other aspects. For example, the path separator will be different.

We should treat PathString as an opaque type as much as possible. The latest patch uses nsLocalFile more for better abstraction. The last `#ifdef XP_WIN` is inevitable because PR_LibSpec_PathnameU is supported only on Windows and PR_LibSpec_Pathname is lossy on Windows.

Comment 8

[Jean-Yves Avenard [:jya]]

Comment on attachment 8950899 [details]
Bug 1425219 - Stop using GetNativePath in ffvpx.

https://reviewboard.mozilla.org/r/220158/#review227062

Comment 9

[Masatoshi Kimura [:emk]]

Comment on attachment 8950899 [details]
Bug 1425219 - Stop using GetNativePath in ffvpx.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220158/diff/2-3/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f60cfdc81e94
Stop using GetNativePath in ffvpx. r=jya

Comment 11

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/f60cfdc81e94