zaps166 / QMPlay2

QMPlay2 is a video and audio player which can play most formats and codecs.
GNU Lesser General Public License v3.0
874 stars 180 forks source link

VTB failure on OS X 10.9 #610

Closed RJVB closed 1 year ago

RJVB commented 1 year ago

As mentioned in my previous ticket, I have a long-standing fails-to-work issue with VTB in QMPlay2 on my Mac (2011 MPB 8.1 running OS X 10.9.5). This hardware acceleration does work in VLC as far as I can tell, so a priori it's not that my OS is too old (VTB was introduced in 10.8 btw).

I've been trying to trace the origin of the problem but get stuck somewhere on what appears to be a C++ problem...

In FFDecVTP::open():

av_hwdevice_ctx_create(&m_hwDeviceBufferRef, AV_HWDEVICE_TYPE_VIDEOTOOLBOX, nullptr, nullptr, 0)

does not fail. The subsequent operation

vtbOpenGL = make_shared<VTBOpenGL>(m_hwDeviceBufferRef);

also appears to succeed (I get a VTBOpenGL instance with its own, non-null copy of m_hwDeviceBufferRef).

Things go wrong in OpenGLWriter::setHWDecContext(): the dynamic cast from the HWDecContext instance to a OpenGLHWInterop fails. With some trace output:

bool OpenGLWriter::setHWDecContext(const shared_ptr<HWDecContext> &hwDecContext)
{
    auto hwInterop = dynamic_pointer_cast<OpenGLHWInterop>(hwDecContext);
    auto hwInterop2 = dynamic_cast<OpenGLHWInterop*>(hwDecContext.get());
    qWarning() << Q_FUNC_INFO << "hwDecContext" << hwDecContext.get()
        << "hwInterop" << hwInterop.get()
        << "hwInterop2" << (void*)hwInterop2;
    if (hwDecContext && !hwInterop)
        return false;

I get virtual bool OpenGLWriter::setHWDecContext(const shared_ptr<HWDecContext> &) hwDecContext 0x11e4061c8 hwInterop 0x0 hwInterop2 0x0

As far as I can tell this makes little sense: VTBOpenGL inherits OpenGLHWInterop inherits HWDecContext so unless something fishy is going on with the shared_ptr implementation a VTBOpenGL instance passed as a hwDecContext pointer should be dynamic-castable to a OpenGLHWInterop pointer.

Any idea how to figure out what goes wrong and why, here?

How crucial is the use of shared_ptr?

RJVB commented 1 year ago

You're not even doing the change about visibility, or the patch in https://github.com/zaps166/QMPlay2/issues/610#issuecomment-1565729830 (which shouldn't hurt anyone else)?

zaps166 commented 1 year ago

You're not even doing the change about visibility

Do you know what's wrong with this? I can remove it, but it'll increase binary files.

or the patch in https://github.com/zaps166/QMPlay2/issues/610#issuecomment-1565729830

it's yours, PR? :smile: Are you sure about version?

RJVB commented 1 year ago

Do you know what's wrong with this? I can remove it, but it'll increase binary files.

Remember that certain dynamic casts failed when hidden visibility was used? ;)

I can confirm it doesn't make a bit difference, but my proposal would be to provide a build option for this. I've seen that in other projects. Never really understood why they chose to give the user a choice in the matter, I guess now I have a better idea. :)

or the patch in https://github.com/zaps166/QMPlay2/issues/610#issuecomment-1565729830

it's yours, PR? :smile: Are you sure about version?

I'm pretty certain about the version, yes, but as I said I have found no hard information about what version of VTB started to handle VP9 and/or on what hardware.

I'll make a combined PR as these changes are all related to VTB support.

zaps166 commented 1 year ago

I'll make a combined PR as these changes are all related to VTB support.

Could you do without visibility?

Remember that certain dynamic casts failed when hidden visibility was used? ;)

My understanding was that the others were working: "Correction, apparently the result can be NULL and is so most of the time, so I completely missed the fact that I did get 2 successful dyncasts with hidden visibility too."

I can change some to qobject_cast - it should be more save. (I can't) :sweat_smile:

zaps166 commented 1 year ago

Do you use Link time optimizations?

RJVB commented 1 year ago

Do you use Link time optimizations?

Sometimes, but not when I want to be able to use a debugger. Which I did with QMPlay2 in order to trace the problem so I can affirm that LTO is not involved here.

What do you mean with "can you do without visibility"? Personally I don't care if you hide all "private" symbols or keep everything visible.

zaps166 commented 1 year ago

Could you do without visibility?

PR for VP9 & other VTB changes, but not touching visibility.

zaps166 commented 1 year ago

It's not VTB, but it's related :smile:

CMAKE_CXX_VISIBILITY_PRESET and CMAKE_VISIBILITY_INLINES_HIDDEN looks nice to control it by CMake, but USE_HIDDEN_VISIBILITY looks like a workaround, and the only person which knows what it can fix is you. Maybe let's do it, but keep it OFF by default for macOS?

zaps166 commented 1 year ago

I want to move FFmpeg lib into libqmplay2 anyway, so the problem might disappear.

RJVB commented 1 year ago

CMAKE_CXX_VISIBILITY_PRESET and CMAKE_VISIBILITY_INLINES_HIDDEN looks nice to control it by CMake, but USE_HIDDEN_VISIBILITY looks like a workaround, and the only person which knows what it can fix is you. Maybe let's do it, but keep it OFF by default for macOS?

Didn't I add a succinct explanation to the option's description? I intended to, and still can.

The CMake presets do work well which could simplify the corresponding portion in your CMakeLists file. We could indeed just dump the option and not use the feature on Mac. I doubt many people will notice...

RJVB commented 1 year ago

Say, sideways related thing: do you try to do anything clever like reusing structures/contexts/channels/whatever when changing (YT) videos mid-play = starting another video while one is already playing? I've had the impression before that things can "get fishy" when you switch (on Linux too). I think I've already lost sound because of that (until I restarted QMPlay2) and just now I had a situation that looked as if VP9 content ended up in a VTB-accelerated pipeline despite the protection I built in.

zaps166 commented 1 year ago

YT videos behaves like local files when switching. What happens exactly? Any errors, logs, etc? VA context is reused, but it depends, audio output is reused if it has the same parameters.

RJVB commented 1 year ago

On Monday June 05 2023 13:43:30 Błażej Szczygieł wrote:

YT videos behaves like local files when switching. What happens exactly?

I get the same trace/bpt trap as when trying to play a VP9 video through VTB. This happens both when I switch from a VP9 video to a H264 one and vice versa, despite seeing the warning message that the content isn't supported by VTB. I could understand this when switching from a video that can be handled by VTB to one that can't; in that case the VA (video accelerated?) context shouldn't be reused. The other way round should be safe but if it crashes that means that somehow some of the "old" VP9 content leaks into the accelerated context.

How hard would it be to stop ongoing playback when switching videos when hardware acceleration is in use?

I had not made a link between the glitches I've noticed in the past when switching videos and the use of HW acceleration but it does seem like a reasonable assumption that there would be a link.

Here's a backtrace. I don't think it will tell you much though and not just because QMplay2 and FFmpeg lack useable debug info (because built with LTO):

Crashed Thread:  0  Dispatch queue: com.apple.main-thread

Exception Type:  EXC_BREAKPOINT (SIGTRAP)
Exception Codes: 0x0000000000000002, 0x0000000000000000

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.CoreFoundation       0x00007fff9356d827 CFGetTypeID + 183
1   com.apple.CoreVideo            0x00007fff9013eb01 _getCVPixelBuffer + 29
2   com.apple.CoreVideo            0x00007fff9013fb81 CVPixelBufferGetIOSurface + 9
3   libFFmpeg.dylib                0x000000011f2e8899 VTBOpenGL::mapFrame(Frame&) + 57
4   libqmplay2.dylib               0x0000000107040ea9 OpenGLCommon::paintGL() + 1641
5   org.qt-project.QtGui           0x00000001098d6175 QPaintDeviceWindowPrivate::paint(QRegion const&) + 117 (qpaintdevicewindow_p.h:96)
6   org.qt-project.QtGui           0x00000001098d5ffc QPaintDeviceWindow::event(QEvent*) + 76 (qpaintdevicewindow_p.h:104)
7   org.qt-project.QtWidgets       0x00000001072a6859 QApplicationPrivate::notify_helper(QObject*, QEvent*) + 265 (qcoreapplication_p.h:100)
8   org.qt-project.QtWidgets       0x00000001072a7ab9 QApplication::notify(QObject*, QEvent*) + 377
9   org.qt-project.QtCore          0x000000010a089d44 QCoreApplication::notifyInternal2(QObject*, QEvent*) + 212 (qcoreapplication.cpp:1031)
10  libqmplay2.dylib               0x0000000107005ce3 OpenGLWindow::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) + 147
11  org.qt-project.QtCore          0x000000010a0b290b QObject::event(QEvent*) + 683 (qobject.cpp:183)
12  org.qt-project.QtWidgets       0x00000001072a6859 QApplicationPrivate::notify_helper(QObject*, QEvent*) + 265 (qcoreapplication_p.h:100)
13  org.qt-project.QtWidgets       0x00000001072a7ab9 QApplication::notify(QObject*, QEvent*) + 377
14  org.qt-project.QtCore          0x000000010a089d44 QCoreApplication::notifyInternal2(QObject*, QEvent*) + 212 (qcoreapplication.cpp:1031)
15  org.qt-project.QtCore          0x000000010a08af16 QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) + 838 (qscopedpointer.h:60)
16  libqcocoa.dylib             0x0000000116e9d922 QCocoaEventDispatcherPrivate::processPostedEvents() + 306 (qcocoaeventdispatcher.mm:899)
17  libqcocoa.dylib             0x0000000116e9e327 QCocoaEventDispatcherPrivate::postedEventsSourceCallback(void*) + 167 (qcocoaeventdispatcher.mm:951)
18  com.apple.CoreFoundation       0x00007fff935c05b1 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
zaps166 commented 1 year ago

This happens both when I switch from a VP9 video to a H264 one and vice versa.

Hmm. Could you try this trick: https://github.com/zaps166/QMPlay2/commit/d62471fbf7a6444dbac9293fa041be23580a3975 https://github.com/zaps166/QMPlay2/commit/bbb5a954e2794ece1df6622c5d040e6fe50603c7 ?

I mean, if you see that codec changes, don't reuse the context. Just remove this if and its contents: https://github.com/zaps166/QMPlay2/blob/a21c8bd0ead5ca6672de990879f28f9394b6fefb/src/modules/FFmpeg/FFDecVTB.cpp#LL97C5-L97C5 and try, if it will fix the crash, I'll make a patch.

RJVB commented 1 year ago

That test was a quick way to force the creation of a new hardware context for every opened video, right? Of course it doesn't work here because the if is never reached in case VTP doesn't support VP9. I tried setting m_hasHWDecContext=false at the start of FFDecVTP::open() but apparently that also doesn't prevent the issue.

The good news is that I appear to have been wrong that switching from VP9 to h264 went wrong too. The last displayed frame from the 1st video does remain visibile despite the changed context so maybe that's what put me on the wrong foot.

RJVB commented 1 year ago

In short, my knowledge of the code isn't sufficient to find a way that resets the hardware context from inside `FFDecVTB::open(). I have tried an unconditional QMPlay2Core.gpuInstance()->setHWDecContextForVideoOutput(nullptr) at the start of the function, assuming that it would be reset to something valid later on, if appropriate. But that gives me a green screen. Something I don't understand either because AFAICT OpenGLWriter::setHWDecContextForVideoOutput() just returns without doing anything when you hand it a nullptr?!

So either there must be some kind of resetHWDecContext() method, or the reset-when-different-codec logic must be implemented somewhere upstream (before the FFDecVTB instance is created?).

Anyway, it seems to me that this issue of switching from a supported codec to an unsupported one must not be exclusive to VP9. Supposing the existing exclusions in the code are still valid on up-to-date hardware and OS versions you should be able to reproduce some for of the issue.. For instance when switching to a H264/YUV420P10 video, or any other content that isn't YUV420P or YUVJ420P.

zaps166 commented 1 year ago

If you switch from VP9 (CPU) to H264 (VTB), VTB is initialized and used.

If you switched from H264 (VTB) to VP9 (CPU), VTB context is released, but it's not clear when it's released. It will be released if all three functions finishes:

and when all Frame instances containing VTB data will be released. QMPlay2 shouldn't free the VTB context before releasing all frames, because every frame holds the VTB reference.

Maybe crash might happen, because I can't see passing VTB context reference inside the VTBOpenGL::mapFrame :thinking: ? (same in VAAPI/OpenGL). It means that VTB context might release before releasing frames. There's no such issue in VAAPI/Vulkan.

zaps166 commented 1 year ago

It means that VTB context might release before releasing frames.

IIRC FFmpeg references the HWAccell context into every frame, so it should be OK (QMPlay2's Frame references the FFmpeg's AVFrame).

zaps166 commented 1 year ago

There's no such issue in VAAPI/Vulkan.

Generally I'm not sure if it's really necessary here :sweat_smile:. Maybe treat it as an additional protection.

zaps166 commented 1 year ago

Could you put qDebug() << videoFrame.hwData(); at the beginning of VTBOpenGL::mapFrame and see what is there before a crash and what is there when operating normally?

zaps166 commented 1 year ago

(on Linux too)

Could you explain? Does it crash, too? What about a sound?

RJVB commented 1 year ago

Can you point me to a H264 video that is YUV420P10 or one that is neither YUV420P nor YUVJ420P? I'd be curious to see what happens if I play that one after playing a H264 video through VTB.

The easiest would still be to do a full stop when the user opens a new video. (That could even be part of the bitperfect feature on Mac as it gives a better guarantee that the audio device is switched back to its original setting. ;) )

Would the user even notice this, aside possibly from the display going black for an instant?

RJVB commented 1 year ago

QMPlay2 shouldn't free the VTB context before releasing all frames, because every frame holds the VTB reference.

Agreed, but can't you let it "get auto-released" when appropriate (to use an ObjC concept) and create a new context for the new video?

zaps166 commented 1 year ago

H264 YUV420P10 shouldn't play on VTB, it's non-standard and it's rejected in QMPlay2. Example to get YUV420P10: ffmpeg -i input.mkv -acodec copy -vcodec libx264 -pix_fmt yuv420p10 out.mkv

RJVB commented 1 year ago

(on Linux too)

Could you explain? Does it crash, too? What about a sound?

No, it happened just often enough to make a link with starting a new video while the previous was still playing. Sorry.

zaps166 commented 1 year ago

No, it happened just often enough to make a link with starting a new video while the previous was still playing. Sorry.

ALSA hw?

Agreed, but can't you let it "get auto-released" when appropriate (to use an ObjC concept)

QMPlay2 is not Obj-C and I don't have access to VTB (FFmpeg is doing the job in VTB). I refrerence FFmpeg's contexts and frames.

and create a new context for the new video?

It's created in your case (CPU->VTB->CPU->VTB). Once you start CPU decoding, VTB context is destroyed. Next you start VTB with a new context (QMPlay2Core.gpuInstance()->getHWDecContext<VTBOpenGL>() returns nullptr).

RJVB commented 1 year ago

Could you put qDebug() << videoFrame.hwData(); at the beginning of VTBOpenGL::mapFrame and see what is there before a crash and what is there when operating normally?

[ass] Using font provider coretext [06 Jun 2023 19:06:59.690] 4993452320 [06 Jun 2023 19:06:59.731] 5007014272 [06 Jun 2023 19:06:59.773] 5007018112 [06 Jun 2023 19:06:59.815] 4997364640 [06 Jun 2023 19:06:59.856] 4997364800 [06 Jun 2023 19:06:59.898] 4997364960 [06 Jun 2023 19:06:59.940] 4997365120

- after switching back to the VP9 video:

[NULL @ 0x12a37bb00] Invalid NAL unit size (6503 > 5688). [NULL @ 0x12a37bb00] missing picture in access unit with size 5692 [06 Jun 2023 19:07:02.600] 4997419872 [06 Jun 2023 19:07:04.671] VP9 not supported by VTB [ass] libass API version: 0x1701000 [ass] libass source: tarball: 0.17.1 [ass] Shaper: FriBidi 0.19.7 (SIMPLE) HarfBuzz-ng 5.1.0 (COMPLEX) [ass] Using font provider coretext [06 Jun 2023 19:07:04.769] 18446744073709551615


The "NULL @" messages usually appear when I interrupt playback, also on Linux), then a frame from the old video still gets through just before `FFDecVTP::open` detects that the new video cannot be played through VTB. The next`videoFrame.hwData()` is clearly not a valid pointer.

How does QMPlay2 handle `VTBOpenGL::mapFrame()` returning false, by aborting playback or by doing a "cold restart"?
zaps166 commented 1 year ago

I know what's wrong. Same in VAAPI/OpenGL (I can crash on Linux, too). VAAPI/Vulkan has a protection and it doesn't crash.


18446744073709551615

Not a valid data. You can guard it by checking if it's Frame::s_invalidCustomData. But the correct solution for what QMPlay2 is doing is here.


I checked ALSA hw (which blocks the audio device) with YT videos - no issues on my PC.

RJVB commented 1 year ago

Bailing out of VTBOpenGL::mapFrame() if videoFrame.hwData() == Frame::s_invalidCustomData prevents the crash and gives a black screen. AFAICT that's also what happens with the correct solution you linked to? Either way, this is preferable ... and it will (probably) cause the user to do a manual cold restart of the video...

zaps166 commented 1 year ago

It happens when you switch from CPU decoded video to VTB, right?

RJVB commented 1 year ago

No, the other way round! Think of it as a backdoor to use VTB on codecs (or formats) that it doesn't support.

zaps166 commented 1 year ago

Switching H264 (VTB) to VP9 causes the issue even with your fix to reject VP9 and with videoFrame.hwData() == Frame::s_invalidCustomData? Is it even possible?

I think I have to start my macOS VM with Radeon, but I guess I'll work for me.

RJVB commented 1 year ago

With

bool VTBOpenGL::mapFrame(Frame &videoFrame)
{
    if (videoFrame.hwData() == Frame::s_invalidCustomData) {
        qWarning() << Q_FUNC_INFO << "called with invalid hwData";
        m_error = true;
        return false;
    }
    CVPixelBufferRef pixelBuffer = (CVPixelBufferRef)videoFrame.hwData();

I now see this on the terminal after switching from H264/VTB to a VP9 video:

[NULL @ 0x1266caa00] Invalid NAL unit size (38698 > 33295).
[NULL @ 0x1266caa00] missing picture in access unit with size 33299
[06 Jun 2023 19:42:36.687] VP9 not supported by VTB
[ass] libass API version: 0x1701000
[ass] libass source: tarball: 0.17.1
[ass] Shaper: FriBidi 0.19.7 (SIMPLE) HarfBuzz-ng 5.1.0 (COMPLEX)
[ass] Using font provider coretext
[06 Jun 2023 19:42:36.778] virtual bool VTBOpenGL::mapFrame(Frame &) called with invalid hwData
[06 Jun 2023 19:42:36.779] OpenGL 2 :: VideoToolBox texture map error
[06 Jun 2023 19:42:36.856] VP9 not supported by VTB
[06 Jun 2023 19:42:36.956] CUVID :: Unable to get function pointers
2023-06-06 19:42:37.113 QMPlay2[27709:e0b] SetNominalSampleRate(48000) setting rate to 48000Hz
2023-06-06 19:42:37.114 QMPlay2[27709:e0b] AudioDevice UMC204HD 192k (73) released

So apparently an attempt to restart the video is made but with hwaccel as a requirement (?!). That fails evidently (2nd VP9 not supported by VTB message), after which CUVID is tried without success. And the QMPlay2 gives up, resets the audio device and calls it quits.

zaps166 commented 1 year ago

When I put if (streamInfo.params->codec_id == AV_CODEC_ID_VP9) return false; in FFDecVAAPI::open, I don't have any issues - VP9 decodes on CPU. Everything from VAAPI is destroyed (tested on OpenGL with and without "render-to-texture" option, too).

I can't just now check on macOS, I have to install a new macOS which supports RX 6900 XT on VM and configure it (GPU passthrough + macOS bootloader to run it properly).

Are you sure you didn't modify anything else? The simplest way is to see if this is working, but you have too old version of macOS, so you can't check.

CUVID

CUVID is not supported on macOS. It should be disabled in CMake.

zaps166 commented 1 year ago

Could you check with current master again?

zaps166 commented 1 year ago

Now added a few more protections.

zaps166 commented 1 year ago

But the correct solution for what QMPlay2 is doing is here.

Doesn't work.

Edit: Fixed for VA-API.

RJVB commented 1 year ago

Took me longer than I thought to refactor my patches but, wow, it works now! I'll do that PR soon, thanks!

zaps166 commented 1 year ago

What if you put __attribute__((visibility("default"))) everywhere in OpenGL and FFmpeg classes/structures/function - does it help?

zaps166 commented 1 year ago

@RJVB ?

RJVB commented 1 year ago

Sorry, I'm not doing much with the current heat but I'll try to look into your question soon.

zaps166 commented 1 year ago

Ok!

RJVB commented 1 year ago

OK, rebuilt with visibility control switched back on globally, and

diff --git a/src/qmplay2/opengl/OpenGLCommon.hpp b/src/qmplay2/opengl/OpenGLCommon.hpp
index 66c871294c740dea4f2ee04e72f08ae1638aa4e0..446c915461215fa8e33c442b535c09bf2fc157b2 100644
--- a/src/qmplay2/opengl/OpenGLCommon.hpp
+++ b/src/qmplay2/opengl/OpenGLCommon.hpp
@@ -37,7 +37,7 @@

 class OpenGLHWInterop;

-class OpenGLCommon : public VideoOutputCommon
+class QMPLAY2SHAREDLIB_EXPORT OpenGLCommon : public VideoOutputCommon
 {
     Q_DECLARE_TR_FUNCTIONS(OpenGLCommon)

diff --git a/src/qmplay2/opengl/OpenGLHWInterop.hpp b/src/qmplay2/opengl/OpenGLHWInterop.hpp
index 85e92c5216529ee120e74080f35f1848e55bee1b..741d7230c76f6ca59bf7ae6f04de87142b50d8f7 100644
--- a/src/qmplay2/opengl/OpenGLHWInterop.hpp
+++ b/src/qmplay2/opengl/OpenGLHWInterop.hpp
@@ -25,7 +25,7 @@

 class ImgScaler;

-class OpenGLHWInterop : public HWDecContext
+class QMPLAY2SHAREDLIB_EXPORT OpenGLHWInterop : public HWDecContext
 {
 public:
     enum Format
diff --git a/src/qmplay2/opengl/OpenGLInstance.hpp b/src/qmplay2/opengl/OpenGLInstance.hpp
index f96b32256ebc570573edad11a8982256578eb6f4..bc58316ef628bf43ba9d378de338e4ae4bc955d1 100644
--- a/src/qmplay2/opengl/OpenGLInstance.hpp
+++ b/src/qmplay2/opengl/OpenGLInstance.hpp
@@ -26,21 +26,31 @@
     #define APIENTRY
 #endif

-class OpenGLInstance final : public GPUInstance
+class QMPLAY2SHAREDLIB_EXPORT OpenGLInstance final : public GPUInstance
 {
 public:
#ifndef OPENGL_ES2
diff --git a/src/qmplay2/opengl/OpenGLWidget.hpp b/src/qmplay2/opengl/OpenGLWidget.hpp
index a2a248b9a6adaccd024dad1282b2ad01bcfba6a7..b351f6b67f831937e16d0c78b7a87f22cf656579 100644
--- a/src/qmplay2/opengl/OpenGLWidget.hpp
+++ b/src/qmplay2/opengl/OpenGLWidget.hpp
@@ -23,7 +23,7 @@
 #include <QSurfaceFormat>
 #include <QOpenGLWidget>

-class OpenGLWidget final : public QOpenGLWidget, public OpenGLCommon
+class QMPLAY2SHAREDLIB_EXPORT OpenGLWidget final : public QOpenGLWidget, public OpenGLCommon
 {
     Q_OBJECT
 public:
diff --git a/src/qmplay2/opengl/OpenGLWindow.hpp b/src/qmplay2/opengl/OpenGLWindow.hpp
index 4245ab8508035e2cb6e5936a5126c0722b32869c..520a0a5fc2298c3cc38e7e0c0ddea2a4da6a1007 100644
--- a/src/qmplay2/opengl/OpenGLWindow.hpp
+++ b/src/qmplay2/opengl/OpenGLWindow.hpp
@@ -22,7 +22,7 @@

 #include <QOpenGLWindow>

-class OpenGLWindow final : private QOpenGLWindow, public OpenGLCommon
+class QMPLAY2SHAREDLIB_EXPORT OpenGLWindow final : private QOpenGLWindow, public OpenGLCommon
 {
     Q_OBJECT
 public:
diff --git a/src/qmplay2/opengl/OpenGLWriter.hpp b/src/qmplay2/opengl/OpenGLWriter.hpp
index 3ef944ce20724c340565633538c81b40403c6a5c..057230bf1f64047afc9f84c551ca82b87a6ef1f6 100644
--- a/src/qmplay2/opengl/OpenGLWriter.hpp
+++ b/src/qmplay2/opengl/OpenGLWriter.hpp
@@ -27,7 +27,7 @@
 class OpenGLHWInterop;
 class OpenGLCommon;

-class OpenGLWriter final : public VideoWriter
+class QMPLAY2SHAREDLIB_EXPORT OpenGLWriter final : public VideoWriter
 {
     Q_DECLARE_TR_FUNCTIONS(OpenGLWriter)
diff --git a/src/modules/FFmpeg/FFDec.hpp b/src/modules/FFmpeg/FFDec.hpp
index a520effb6b9b1ef3ee95c51171340f956e43d5a5..58f182e555a7e651b3546d588ef556f372eaed53 100644
--- a/src/modules/FFmpeg/FFDec.hpp
+++ b/src/modules/FFmpeg/FFDec.hpp
@@ -23,6 +23,18 @@
 #include <QString>
 #include <QList>

+#include <QtGlobal>
+
+#ifdef Q_OS_MACOS
+    #if defined(QMPlay2_VTB)
+        #define FFMPEGVTB_EXPORT Q_DECL_EXPORT
+    #else
+        #define FFMPEGVTB_EXPORT Q_DECL_IMPORT
+    #endif
+#else
+    #define FFMPEGVTB_EXPORT /**/
+#endif
+
 #ifdef USE_VULKAN
 namespace QmVk {
 class ImagePool;
@@ -34,7 +46,7 @@ struct AVPacket;
 struct AVCodec;
 struct AVFrame;

-class FFDec : public Decoder
+class FFMPEGVTB_EXPORT FFDec : public Decoder
 {
 protected:
     FFDec();
diff --git a/src/modules/FFmpeg/FFDecHWAccel.hpp b/src/modules/FFmpeg/FFDecHWAccel.hpp
index f6ba032281644410e07109ce6440ff583bff6888..0f17017c3405a298d4ba9657160bd92d4d13ee14 100644
--- a/src/modules/FFmpeg/FFDecHWAccel.hpp
+++ b/src/modules/FFmpeg/FFDecHWAccel.hpp
@@ -20,7 +20,7 @@

 #include <FFDec.hpp>

-class FFDecHWAccel : public FFDec
+class FFMPEGVTB_EXPORT FFDecHWAccel : public FFDec
 {
 protected:
     FFDecHWAccel();
diff --git a/src/modules/FFmpeg/FFDecSW.hpp b/src/modules/FFmpeg/FFDecSW.hpp
index a3aa8909add8aae3e3fa05279b48995ec76127af..c2eff8f568b5cce1a1c639f59477d4444ab55742 100644
--- a/src/modules/FFmpeg/FFDecSW.hpp
+++ b/src/modules/FFmpeg/FFDecSW.hpp
@@ -49,7 +49,7 @@ public:

 struct SwsContext;

-class FFDecSW final : public FFDec
+class FFMPEGVTB_EXPORT FFDecSW final : public FFDec
 {
 public:
     FFDecSW(Module &);
diff --git a/src/modules/FFmpeg/FFDecVTB.hpp b/src/modules/FFmpeg/FFDecVTB.hpp
index f6f4d547b301eeab9299d0b096273616fc25a5f8..d47eede2379a5defa9be0af06cad8c7e298a44ab 100644
--- a/src/modules/FFmpeg/FFDecVTB.hpp
+++ b/src/modules/FFmpeg/FFDecVTB.hpp
@@ -20,7 +20,7 @@

 #include <FFDecHWAccel.hpp>

-class FFDecVTB final : public FFDecHWAccel
+class FFMPEGVTB_EXPORT FFDecVTB final : public FFDecHWAccel
 {
 public:
     FFDecVTB(Module &module);

That works too, AFAICT. The install image is marginally smaller:

-rw-r--r-- 1 root admin 1268612 Jun  7 03:27 qmplay2-devel-23.06.04.8_0+LTO+langselect+qt5kde.darwin_13.x86_64.txz
-rw-r--r-- 1 root admin 1218960 Jun 15 18:54 qmplay2-devel-23.06.04.8_1+LTO+langselect+qt5kde.darwin_13.x86_64.txz
RJVB commented 1 year ago

NB: in short I added QMPLAY2SHAREDLIB_EXPORT to all headers in the OpenGL subdir. In the FFmpeg extension, I define FFMPEGVTB_EXPORT to the appropriate __attribute__ on Mac, and to an empty comment elsewhere, and add the attribute to every child of FFDec.

zaps166 commented 1 year ago

Thanks, I'd prefer FFMPEG_EXPORT instead. Now let's guess which are necessary (I guess not all) and make a PR to finally fix it :smile: (sorry, I can't reproduce, so can't check it).

First check if only-FFMpeg or only-OpenGL is enough. Next check which classes needs them and make PR with export added only where needed (for all OSes of course, no ifdefs) :slightly_smiling_face:

RJVB commented 1 year ago

(for all OSes of course, no ifdefs) :slightly_smiling_face:

There has to be at least 1 ifdef, of course (to determine between import and export), so I'd need introduce a new flag if what I do currently isn't to your liking.

Would you want this attribute applied to all different hwaccel types?

zaps166 commented 1 year ago

There has to be at least 1 ifdef,

Yes :sweat_smile: Just to not make ifdef only for macOS.

Would you want this attribute applied to all different hwaccel types?

Yes, but I can handle it, too.


Just copy file with QMPLAY2SHAREDLIB_EXPORT into FFmpeg module, change define name, and use only where it its needed :smile:

RJVB commented 1 year ago

ping?

zaps166 commented 1 year ago

I know, I'll merge it later!

RJVB commented 1 year ago

Okidoki! :)