Stop using GetNativePath in ffvpx

RESOLVED FIXED in Firefox 60

Status

()

defect
P3
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: emk, Assigned: emk)

Tracking

unspecified
mozilla60
Unspecified
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

2 years ago
No description provided.
Priority: -- → P3
Assignee

Comment 1

a year ago
Attached a patch in bug 685236.
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → DUPLICATE
Duplicate of bug: 685236
Assignee

Updated

a year ago
Blocks: 1428258
Assignee

Comment 2

a year ago
I'll move ffvpx part from bug 685236 to this bug.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Assignee

Comment 3

a year ago
(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 hidden (mozreview-request)
Assignee

Updated

a year ago
Assignee: nobody → VYV03354
Status: REOPENED → ASSIGNED

Comment 5

a year ago
mozreview-review
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?
Attachment #8950899 - Flags: review?(jyavenard) → review-
Comment hidden (mozreview-request)
Assignee

Comment 7

a year ago
mozreview-review-reply
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

a year ago
mozreview-review
Comment on attachment 8950899 [details]
Bug 1425219 - Stop using GetNativePath in ffvpx.

https://reviewboard.mozilla.org/r/220158/#review227062
Attachment #8950899 - Flags: review?(jyavenard) → review+
Comment hidden (mozreview-request)

Comment 10

a year ago
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/f60cfdc81e94
Stop using GetNativePath in ffvpx. r=jya

Comment 11

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f60cfdc81e94
Status: ASSIGNED → RESOLVED
Last Resolved: a year agoa year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.