Closed Bug 1471581 Opened 3 years ago Closed 3 years ago

Fix libvpx mingw clang compilation.

Categories

(Core :: Audio/Video: Playback, enhancement, P5)

enhancement

Tracking

()

RESOLVED INVALID

People

(Reporter: jacek, Assigned: jacek)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

The situation here is tricky. Current mingw config files define HAVE_PTHREAD_H to 1. This does not work with mingw clang toolchain that does not have winpthreads (we could add it, but we don't need it so I'd rather leave it out). HAVE_PTHREAD_H was explicitly set in bug 1406542 for mingw 32-bit GCC-based toolchain that has winpthreads. It was added due to warnings, so maybe it's not too bad to switch it back and accept those workings... The alternative is to introduce #ifdef like (this would keep HAVE_PTHREAD_H as 1 for GCC builds):
#ifndef HAVE_PTHREAD_H
#define HAVE_PTHREAD_H 0
#endif

or #undef and have it 0 for both platforms:
#undef HAVE_PTHREAD_H
#define HAVE_PTHREAD_H 0

Both solutions touch generated files, so it's not pretty. The patch I will send just sets it to 0, meaning that it will introduce warnings on GCC. It also touches only 64-bit configs, which reveals one more problem. Bug 1448453 introduced 64-bit config, but didn't include generate_sources_mozbuild.sh changes. I did those changes and reran the script.


Yet another solution would be to use msvc config. As far as I can tell, it almost works with clang. The only change it would need is to use inline instead of __forceinline for INLINE macro.
Attachment #8988149 - Flags: feedback?(tom)
Comment on attachment 8988149 [details]
Bug 1471581 - Fix libvpx mingw clang compilation.

(In reply to Jacek Caban from comment #0)
> It was added due to warnings

There were warnings; but I think it affected functionality too.  What I got from https://bugzilla.mozilla.org/show_bug.cgi?id=1406542#c3 was that the libraries would use HAVE_PTHREAD_H to determine which backing API to use for their own pthread emulation. And then Ralph conjectured that it would probably be better to use pthreads (if available) rather than whatever else they used.

Assuming our conjectures are right (and you're probably better suited to judge than we are!) I'm inclined to have an #ifdef that makes it 1 for mingw-gcc and 0 for mingw-clang.

We have the sed -i lines in generate_sources_mozbuild.sh - that is probably enough to accomodate this? (And we would need to add those for mingw64 also I think...)
Attachment #8988149 - Flags: feedback?(tom)
Does the new version look better? It will use mozilla-config.h version if available and default to 0 otherwise. Because of further check for .asm and .h consistency, .asm version has to be 0, but it's not used anywhere anyway.
(In reply to Jacek Caban from comment #4)
> Does the new version look better? It will use mozilla-config.h version if
> available and default to 0 otherwise. Because of further check for .asm and
> .h consistency, .asm version has to be 0, but it's not used anywhere anyway.

Well, there's the .asm HAVE_PTHREAD constants - how much do those matter?  Aside from that, LGTM.
I think that such ifdef is not possible in assembly version, but as far as I can see assembly define is never used, so it shouldn't matter.
Component: Audio/Video → Audio/Video: Playback
Priority: -- → P5
Comment on attachment 8988149 [details]
Bug 1471581 - Fix libvpx mingw clang compilation.

https://reviewboard.mozilla.org/r/253406/#review260308

I prefer to not take on that one, not knowing the performance impact that may be introduced should pthread be disabled on windows
Attachment #8988149 - Flags: review?(jyavenard)
Comment on attachment 8988149 [details]
Bug 1471581 - Fix libvpx mingw clang compilation.

https://reviewboard.mozilla.org/r/253406/#review260308

To be more accurate, I don't want to see performance degradations on default windows for the sake of improving support for a tier.2 (or is that tier.3) platform.
(In reply to Jean-Yves Avenard [:jya] from comment #8)
> Comment on attachment 8988149 [details]
> Bug 1471581 - Fix libvpx mingw clang compilation.
> 
> https://reviewboard.mozilla.org/r/253406/#review260308
> 
> To be more accurate, I don't want to see performance degradations on default
> windows for the sake of improving support for a tier.2 (or is that tier.3)
> platform.

Sorry, I think there's a misunderstanding.  This config only affects the MinGW builds, it doesn't affect any other platform. The conversation Jacek and I were having was about the mingw-gcc build (which Tor currently uses) and the mingw-clang build (which Tor will switch too sometime in the next year.)

This config and patch also shouldn't affect the mingw-gcc performance at all: we're keeping the existing behavior using an #ifdef.
Flags: needinfo?(jyavenard)
I just found that on m-c, there is a commit from bug 1453061 that solves __forceinline problem I mentioned in comment 1. With that commit, we may just use the same config for clang as we use for msvc and my patch is not needed.
Depends on: 1453061
(In reply to Tom Ritter [:tjr] from comment #9)
> (In reply to Jean-Yves Avenard [:jya] from comment #8)
> > Comment on attachment 8988149 [details]
> > Bug 1471581 - Fix libvpx mingw clang compilation.
> > 
> > https://reviewboard.mozilla.org/r/253406/#review260308
> > 
> > To be more accurate, I don't want to see performance degradations on default
> > windows for the sake of improving support for a tier.2 (or is that tier.3)
> > platform.
> 
> Sorry, I think there's a misunderstanding.  This config only affects the
> MinGW builds, it doesn't affect any other platform. The conversation Jacek
> and I were having was about the mingw-gcc build (which Tor currently uses)
> and the mingw-clang build (which Tor will switch too sometime in the next
> year.)
> 
> This config and patch also shouldn't affect the mingw-gcc performance at
> all: we're keeping the existing behavior using an #ifdef.

You're right, my bad.... I thought this was modifying all config.

Apologies
Flags: needinfo?(jyavenard)
This is no longer needed.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.