Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

macOS opengl-cb backend #4833

Merged
merged 3 commits into from Feb 12, 2018
Merged

macOS opengl-cb backend #4833

merged 3 commits into from Feb 12, 2018

Conversation

Akemi
Copy link
Member

@Akemi Akemi commented Sep 4, 2017

this is a WIP and attempts to fix various annoying macOS bugs and the very long standing fullscreen regression completely. it still needs testing and some fix-ups.

this will definitely fix: #5478, #5393, #5152, #5151, #4615, #4476, #3746, #3739, #2392, #2217
and possibly fixes: #3978

feel free to test and report unknown bugs. for now one just needs to set --vo=opengl-cb, though if the mpv core is too fast you might get a "No context set." error.

there are a few things to do where i don't know how to solve them yet and i might need some help.
TODO cocoa-cb:

  • proper cocoa-cb init routine, (--no-config --vo=opengl-cb fails since no context yet)
  • cocoa-cb vo option or comparable (just compile time flag, uses vo=opengl-cb)
  • doesn't draw when minimised, off screen, different space etc.
  • get a hide (and unhide) event for windowless playback
  • remove "[opengl-cb] icc-profile-auto is not available with opengl-cb" warning in an acceptable way
  • change lux scale (db0fb3c)
  • remove "Taking screenshot failed." warning, appears on screenshot (window), not too happy with the window screenshot code
  • on shutdown wait for animations to finish
  • performance data buggy
  • ICC profile switching leads to flicker in some cases (with icc-profile-auto=no, most likely an Apple bug)

TODO Build:

  • flag to deactivate cocoa-cb
  • dynamic developer path for static_swift
  • set build target min system requirements

things i will fix soonish:

  • fullscreen animation stretches video to target aspect ratio
  • crashes on video stream switch to no-video (related to "hide and unhide event for windowless")
  • implement none-native fs
  • add license headers
  • (deactivate the early flush on macOS)

TODO misc:

  • add a deprecation warning for the old opengl cocoa backend?

int r = VO_NOTIMPL;
if (p->ctx->control_cb) {
int events = 0;
r = p->ctx->control_cb(p->ctx->control_cb_ctx, &events, request, data);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsynchronized access to the callback.

write_screenshot(mpctx, img, filename, NULL, true);
talloc_free(filename);
talloc_free(img);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really like this approach, and also synchronization of any form is missing.

@AirPort
Copy link

AirPort commented Sep 4, 2017

As I said in the previous issue, I tested it for a bit and so far I didn't notice any particular difference performance wise compared to master, and it definitely seems to fix the mentioned issues (not 100% sure about #3978, only time and use will tell since it's difficult to reproduce at will).
I'll keep on testing and report back if I notice something noteworthy.

Just one question though: during testing I once forgot to type --vo=opengl-cb, so it started with regular vo=opengl and hanged without drawing any window. Are the two cocoa backends mutually exclusive or in the final version will be still possible to select and use the current one (e.g. for testing purposes)?

@Akemi
Copy link
Member Author

Akemi commented Sep 4, 2017

that will be fixed in the final version. it's just a matter of a proper option and updating the init routine of it (a little bit). after that using both will be possible.

@JCount
Copy link
Contributor

JCount commented Sep 6, 2017

This may not be useful, but on OS X 10.9 and later, the CGLOpenGLProfile kCGLOGLPVersion_GL4_Core could potentially be useful in instances where the cocoa-cb would want (the system to) /* choose a renderer capable of GL4.1 or later */.

@Akemi
Copy link
Member Author

Akemi commented Sep 6, 2017

kCGLOGLPVersion_3_2_Core is just a misnomer atm and just means request the highest possible version but at least 3.2. so yeah kCGLOGLPVersion_GL4_Core isn't useful at all atm.

@JCount
Copy link
Contributor

JCount commented Sep 6, 2017

Ok, I wasn't sure if some of the dual GPU systems could have only one GPU capable of OpenGL 4.1, which if not already selected as the default, could be switched over by the system if this were set.? Essentially, would it ever be desirable to raise the minimum bound that high for the CGLOpenGLProfile. 😄

@ottob
Copy link

ottob commented Sep 9, 2017

Thanks for working on this. I tried to build d5c93df on high sierra (17A360a) but it fails with:

Waf: Leaving directory `/Users/ottob/Documents/Develop/mpv-build/mpv/build'
Build failed
 -> task in 'osdep/macOS_swift.o osdep/macOS_swift.h osdep/macOS_swift.swiftmodule' failed with exit status 1 (run with -v to display more information)

Any ideas?

@Akemi
Copy link
Member Author

Akemi commented Sep 9, 2017

if you can run the build with -v and post that log it would be helpful.

@AirPort
Copy link

AirPort commented Sep 9, 2017

It's the same error I had when I tried to build it without the full Xcode.
You can try a temporary workaround by manually changing the
'-L/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/swift_static/macosx'
line in wscript_build.py to
'-L/Library/Developer/CommandLineTools/usr/lib/swift_static/macosx'
and see if it builds.

@ottob
Copy link

ottob commented Sep 10, 2017

Thanks for the hint AirPort. I modified wscript_build.py file like so (I have xcode 9 beta installed)

-            '-L/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/swift_static/macosx',
+            '-L/Applications/Xcode-beta.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/swift_static/macosx',

But still it wont build, looks like it might be Swift 4 errors. I have put the verbose log here: https://gist.github.com/ottob/11648da514c0986f72b2c7a60fceb9cf

@macdavis
Copy link

It seems it just produces a black screen when mpv's dithering is enabled. Tested with d5c93df.
Log file: http://sprunge.us/YARh

@AirPort
Copy link

AirPort commented Sep 10, 2017

I can reproduce the black screen, but only with --dither-size-fruit=8. Any other value (included default 6) seems to work fine.

@Akemi
Copy link
Member Author

Akemi commented Sep 10, 2017

@ottob yeah i tested this with swift 3. you can add a --swift-version 3 here, behind the -O. that should work, couldn't test it myself.

@macdavis dithering is always enabled by default. it's just the size of the fruit dither you used. it already takes ages to load here with the normal cocoa backend. it also does work with cocoa-cb you just have to wait (the same time) till the video appears. the difference is that with the normal cocoa backend the mpv core blocks till it's done computing, with opengl-cb it can already start drawing without the blocking, that results in a temporary black screen.

@macdavis
Copy link

Thanks! The video does show after some time of waiting.

@Akemi
Copy link
Member Author

Akemi commented Sep 10, 2017

@macdavis i reported your problem as a separate issue #4852, since it's a problem all opengl-cb users face.

@ottob
Copy link

ottob commented Sep 13, 2017

yeah i tested this with swift 3. you can add a --swift-version 3 here, behind the -O. that should work, couldn't test it myself.

-swift-version 3 single dash worked, thanks.

@Akemi
Copy link
Member Author

Akemi commented Sep 13, 2017

indeed a single dash. i blame mpv for that.

@ottob
Copy link

ottob commented Sep 13, 2017

Ive only done some very basic testing, but have not noticed any regressions. Thanks for maintaining the macOS code :)

Some debug prints are still around, but you probably know that:

cocoa_set_mpv_handle start
setMpvHandle start
copyCGLPixelFormat start-----
copyCGLPixelFormat end-----
copyCGLContext------
setMpvHandle end
cocoa_set_mpv_handle end

@@ -479,7 +484,13 @@ static int reconfig(struct vo *vo, struct mp_image_params *params)

void mp_client_set_icc_profile(struct mpv_opengl_cb_context *ctx, bstr icc_data)
{
pthread_mutex_lock(&ctx->icc_lock);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to always call the function on the renderer thread? That's probably also how an official opengl-cb API for this would work.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh do you mean like it was before (thought the current approach was preferable) or do it in the mpv_opengl_cb_draw function via a flag?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, just rule that mp_client_set_icc_profile can be called in the GL thread only, so that no additional synchronization is needed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure can do that. something broke anyway when i rebased it with the current "vo_gpu" changes. just getting a "blue screen" and shader errors on some icc profile changes.

should i mention that rule somewhere?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a comment on the function would be good.

@kevmitch
Copy link
Member

If ever this emoji was justified.

@maru-sama
Copy link

Thank you very much @Akemi for the work you put in that PR and especially for making mpv a first class citizen in the every changing Apple environment again.
See you again when 10.14 hits the streets and Apple decides to change APIs again. :)

@rien333
Copy link

rien333 commented Feb 12, 2018

When I try to upgrade from brew, I get this:

$ brew upgrade mpv
...
==> python -c import setuptools... --no-user-cfg install --prefix=/private/tmp/mpv-20180212-94919-k8ftgy/vendor --single-version-externally-managed --record=installed.txt
==> ./bootstrap.py
==> python3 waf configure --prefix=/usr/local/Cellar/mpv/HEAD-c5e4538 --enable-zsh-comp --enable-libmpv-shared --enable-html-build --enable-lua --confdir=/usr/local/etc/mpv --datadir=/usr/local/Cellar/mpv/HEAD-c5e4538/share/mpv --mandir=/
...
File "/private/tmp/mpv-20180212-94919-k8ftgy/waftools/detections/compiler_swift.py", line 17, in __add_swift_flags
    swift_version = __run(ctx.env.SWIFT +  ' -version').split(' ')[3].split('.')
TypeError: can't concat str to bytes

Might be an error on their side, but maybe good to know anyway.

@maru-sama
Copy link

maru-sama commented Feb 12, 2018

Damn, did not test it with python3 in my setup. Can you try if b' -version' fixes this?
This is just a quick fix, though.

@Akemi
Copy link
Member Author

Akemi commented Feb 12, 2018

yeah i am in the process of fixing a few issues.

@rien333
Copy link

rien333 commented Feb 12, 2018

@maru-sama I would love to, but I'm not sure how to change brew's compilation files. There is a git folder in ~/Library/Caches/Homebrew/mpv--git/ where I can change the compiler_swift.py file, but it seems like brew copies this git cache folder to a different folder in /tmp whenever it tries to build from source, and also overwrites my local changes.

@maru-sama
Copy link

Forget it, this won't work anyway since split is used afterwards which will obviously fail. Akemi is working on it right now.

@rien333
Copy link

rien333 commented Feb 12, 2018

Thanks for fixing the python3 issue for the configuration step! Unfortunately, brew still fails in the linking/compiling step:

$ brew upgrade mpv
...
==> ./bootstrap.py
==> python3 waf configure --prefix=/usr/local/Cellar/mpv/HEAD-8762818 --enable-zsh-comp --enable-libmpv-shared --enabl
==> python3 waf install
Last 15 lines from /Users/rw/Library/Logs/Homebrew/mpv/04.python3:
[435/445] Compiling input/event.c
[436/445] Compiling input/ipc.c
[437/445] Compiling misc/thread_pool.c
[438/445] Compiling video/out/opengl/common.c
[439/445] Linking build/mpv
[440/445] Linking build/libmpv.dylib
Undefined symbols for architecture x86_64:
  "_OBJC_CLASS_$__TtC11macOS_swift7CocoaCB", referenced from:
      objc-class-ref in macosx_application.m.26.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

@Akemi
Copy link
Member Author

Akemi commented Feb 12, 2018

i believe it's because cocoa-cb is statically linked to the swift libs. for now either build with --enable-libmpv-static and not --enable-libmpv-shared, or disable cocoa-cb --disable-macos-cocoa-cb. i will try to fix that as fast as possible.

@rien333
Copy link

rien333 commented Feb 12, 2018

Okay, thanks for your work. I'll try to edit the homebrew formula to see if I can pass those flags.
EDIT:
Editing the homebrew formula to produce a static instead of shared build of libmpv did indeed work, and the formula now builds without a problem.

@Akemi
Copy link
Member Author

Akemi commented Feb 12, 2018

a quick look, it seems like the problem is that it doesn't try to link to the swift object file when libmpv is linked. maybe my first guess was wrong too.

@rien333
Copy link

rien333 commented Feb 12, 2018

Another question, is it intentional that $ mpv --vo=help does not show opengl-cb, even though passing it as the vo does work?

@Akemi
Copy link
Member Author

Akemi commented Feb 12, 2018

yeah it's explicitly filtered out here. is this a problem and should be changed? it's like this since opengl-cb was only meant to be used with libmpv.

@deus0ww
Copy link

deus0ww commented Feb 12, 2018

Using vo=opengl-cb, the stats script (shift-i), does not show anything on the second page. This is with MPV HEAD (8762818) with --enable-libmpv-static. Using vo=gpu, the second page works as expected. Is this expected?

@Akemi
Copy link
Member Author

Akemi commented Feb 12, 2018

yes this is a known limitation of opengl-cb. i added the functionality but it was buggy and i had to remove it for now, also see #5322. i will (re)add it with a future PR, i already have a working branch.

@AirPort
Copy link

AirPort commented Feb 12, 2018

I've been busy the last couple of days so I couldn't test it in its final version, but now that it's finally been merged I rushed to build it. Unfortunately it seems that commit 235eb60 broke the ffmpeg detection check on my end (macOs 10.13.3).
Up until that commit it works fine

Checking for FFmpeg/Libav present                                    : yes 
Checking for libav* is FFmpeg                                        : yes 
Checking for libav* is Libav                                         : no 
Checking for Libav/FFmpeg library versions                           : yes 

since 235eb60 waf gives an error and then it stops

Checking for FFmpeg/Libav present                                    : yes 
Checking for libav* is FFmpeg                                        : no 
Checking for libav* is Libav                                         : no 
Checking for Libav/FFmpeg library versions                           : ffmpeg not found 
Unable to find development files for some of the required FFmpeg/Libav libraries. Git master is recommended.

(of course it's the latest ffmpeg git version in both cases). Any idea?

@Akemi
Copy link
Member Author

Akemi commented Feb 12, 2018

i will try too look at it tomorrow. seems kinda weird though.

@AirPort
Copy link

AirPort commented Feb 12, 2018

If it can be useful, this is the waf config.log file (and here's the one for the last working commit, abf2efb).

@Akemi
Copy link
Member Author

Akemi commented Feb 13, 2018

@AirPort i see this

ld: library not found for -ltheoraenc
clang: error: linker command failed with exit code 1 (use -v to see invocation)

can you maybe rebuild ffmpeg and if this doesn't work addtiotnally disable libtheoraenc.

could be that libtheoraenc was updated without ffmpeg being rebuild or something.

@AirPort
Copy link

AirPort commented Feb 13, 2018

I'll try and report, but the strange thing about that is that I'm trying to build mpv against the exact same static built ffmpeg's libraries in both cases.

BTW, what about

err: �[1m../test.c:3:9: �[0m�[0;1;31merror: �[0m�[1m'x' declared as an array with a negative size�[0m
{ int x[LIBAVCODEC_VERSION_MICRO >= 100 ? -1 : 1]; return 0; }
�[0;1;32m        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
�[0m�[1m../../../../../Applications/mpv-build/build_libs/include/libavcodec/version.h:32:34: �[0m�[0;1;30mnote: �[0mexpanded from macro 'LIBAVCODEC_VERSION_MICRO'�[0m
#define LIBAVCODEC_VERSION_MICRO 100
�[0;1;32m                                 ^
�[0m1 error generated.

a few lines beneath?

@Akemi
Copy link
Member Author

Akemi commented Feb 13, 2018

that's the libav check and is supposed to fail, in your case. it also fails in both of your logs.

@AirPort
Copy link

AirPort commented Feb 13, 2018

Oh crap, you're right.
Just rebuilt ffmpeg without libtheora though, same error.

Maybe tomorrow I'll try with shared ffmpeg, maybe the issue is with static libraries?

@Akemi
Copy link
Member Author

Akemi commented Feb 13, 2018

the same error shouldn't happen if nothing else depends on it or it picks up an older version of ffmpeg. could you maybe try brew uninstall --force ffmpeg and rebuilt?

maybe also try removing line 106-109, the whole if. i just tried building on 10.13 too and it worked for me.

@AirPort
Copy link

AirPort commented Feb 13, 2018

So, I made it work by removing the the --enable-libsoxr option from ffmpeg build (toghether with --enable-libtheora). I don't know why the presence of those two options suddenly caused the build to fail between those two commit i referenced before, but I honestly think I can live without them. If you still want to investigate more on the matter, I'll be glad to provide any useful information tomorrow.
And to be completely clear on how I build mpv, to maybe better investigate on the issue: while I use brew to install the various dependency, I build mpv and git ffmpeg for it via mpv-build (that's why static ffmpeg).

Anyways, good job on the cocoa-cb backend! Seeing all those annoying little issue going away for good is just great.

@Akemi Akemi mentioned this pull request Feb 13, 2018
@Akemi
Copy link
Member Author

Akemi commented Feb 13, 2018

i still can't reproduce the ffmpeg linking issue. my ffmpeg is also build with libtheora and libsoxr. both are the bottled versions from current homebrew.

@deus0ww
Copy link

deus0ww commented Feb 13, 2018

To provide another data point, I have no problem installing homebrew ffmpeg with:
"--with-openssl --with-rtmpdump --with-libass --with-rubberband --with-libsoxr --with-game-music-emu --with-libgsm --with-libvorbis --with-libvpx --with-openjpeg --with-opus --with-speex".

@AirPort
Copy link

AirPort commented Feb 13, 2018

Well, I honestly don't care for those two options enough to make you lose your time chasing an elusive issue for basically no gain.
I'll try again with those option enabled with mpv-build as soon as the build fixes' PR is merged (to make things as clean as possibile), and if it still don't work I'd say to blame static build's shenanigans and just to ignore it unless more people start to report issues about it.

@Akemi
Copy link
Member Author

Akemi commented Feb 13, 2018

if it's a widespread issue i bet we will get a lot more complains from homebrew and more input on the source of the problem. i will probably investigate it at that point some more. sry we only have a workaround for that atm.

@Akemi Akemi deleted the macOS_cocoa-cb2 branch February 14, 2018 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Delay on startup Framedrop in macOS after leaving mpv in the background