Remove ofQTKitPlayer, set OS X 10.8 as the minimum #3543

pizthewiz · 2015-01-11T00:50:48Z

~~REQUIRES FURTHER TESTING BEFORE A CANDIDATE FOR MERGE~~

With OS X 10.8 as the minimum, the AV Foundation player is now the default and in lieu of an AV Foundation grabber, we are still using the QTKit grabber. This does not fully remove 10.6 or 10.7 support, there are various checks sprinkled around that still reference 10.7, this only realigns the video player and grabber.

I have tested the video examples on OS X 10.10 and all should also be tested on OS X 10.9 and 10.8 as well. I have both in a VM, I'll give that a try soon, though I suspect all of the capture variants will sadly not work - will see soon.

pizthewiz · 2015-01-12T18:44:44Z

It turns out the built-in iSight camera can be exposed into the VM and I was able to test the 3 video examples that use the QTKit grabber.

Testing Coverage

[✔️] OS X 10.10.2 / Xcode 6.1.1 / Xcode 6.2 B3
[✔️] OS X 10.9.5 / Xcode 5.1.1 (via VMware Fusion)
[✔️] OS X 10.8.5 / Xcode 5.1.1 (via VMware Fusion)

Building via make has not been tested.

pizthewiz · 2015-01-12T18:50:47Z

It appears as though the makefiles are not linking to the AVFoundation and CoreMedia frameworks. That isn't something that was introduced in this PR, so I'm not entirely sure if we should fix that here or not. @bakercp is a separate PR cool for that tweak?

% make
[…]
Linking bin/videoPlayerExample for osx
c++ -o bin/videoPlayerExample  obj/osx/Release/src/main.o obj/osx/Release/src/ofApp.o  ../../../libs/openFrameworksCompiled/lib/osx/libopenFrameworks.a   -stdlib=libstdc++ -arch i386 -F../../../libs/glut/lib/osx/ -mmacosx-version-min=10.8 -framework Accelerate -framework QTKit -framework GLUT -framework AGL -framework ApplicationServices -framework AudioToolbox -framework CoreAudio -framework CoreFoundation -framework CoreServices -framework OpenGL -framework QuickTime -framework IOKit -framework Cocoa -framework CoreVideo ../../../libs/FreeImage/lib/osx/freeimage.a ../../../libs/cairo/lib/osx/cairo-script-interpreter.a ../../../libs/cairo/lib/osx/cairo.a ../../../libs/cairo/lib/osx/pixman-1.a ../../../libs/freetype/lib/osx/freetype.a ../../../libs/glew/lib/osx/glew.a ../../../libs/glfw/lib/osx/libglfw3.a ../../../libs/openssl/lib/osx/crypto.a ../../../libs/openssl/lib/osx/ssl.a ../../../libs/poco/lib/osx/PocoCrypto.a ../../../libs/poco/lib/osx/PocoData.a ../../../libs/poco/lib/osx/PocoDataODBC.a ../../../libs/poco/lib/osx/PocoDataSQLite.a ../../../libs/poco/lib/osx/PocoFoundation.a ../../../libs/poco/lib/osx/PocoNet.a ../../../libs/poco/lib/osx/PocoNetSSL.a ../../../libs/poco/lib/osx/PocoUtil.a ../../../libs/poco/lib/osx/PocoXML.a ../../../libs/poco/lib/osx/PocoZip.a ../../../libs/rtAudio/lib/osx/rtAudio.a ../../../libs/tess2/lib/osx/tess2.a  ../../../libs/fmodex/lib/osx/libfmodex.dylib  -lobjc 
Undefined symbols for architecture i386:
  ".objc_class_name_AVPlayer", referenced from:
      pointer-to-literal-objc-class-name in libopenFrameworks.a(ofAVFMovieRenderer.o)
  ".objc_class_name_AVPlayerItem", referenced from:
      pointer-to-literal-objc-class-name in libopenFrameworks.a(ofAVFMovieRenderer.o)
  ".objc_class_name_AVPlayerItemVideoOutput", referenced from:
      pointer-to-literal-objc-class-name in libopenFrameworks.a(ofAVFMovieRenderer.o)
  ".objc_class_name_AVURLAsset", referenced from:
      pointer-to-literal-objc-class-name in libopenFrameworks.a(ofAVFMovieRenderer.o)
  "_AVMediaTypeVideo", referenced from:
      ___28-[AVFMovieRenderer loadURL:]_block_invoke_2 in libopenFrameworks.a(ofAVFMovieRenderer.o)
  "_AVPlayerItemDidPlayToEndTimeNotification", referenced from:
      ___28-[AVFMovieRenderer loadURL:]_block_invoke_2 in libopenFrameworks.a(ofAVFMovieRenderer.o)
  "_CACurrentMediaTime", referenced from:
      -[AVFMovieRenderer update] in libopenFrameworks.a(ofAVFMovieRenderer.o)
  "_CMTimeGetSeconds", referenced from:
      -[AVFMovieRenderer duration] in libopenFrameworks.a(ofAVFMovieRenderer.o)
      -[AVFMovieRenderer currentTime] in libopenFrameworks.a(ofAVFMovieRenderer.o)
  "_CMTimeMakeWithSeconds", referenced from:
      -[AVFMovieRenderer setCurrentTime:] in libopenFrameworks.a(ofAVFMovieRenderer.o)
  "_CMTimeMaximum", referenced from:
      -[AVFMovieRenderer setCurrentTime:] in libopenFrameworks.a(ofAVFMovieRenderer.o)
  "_CMTimeMinimum", referenced from:
      -[AVFMovieRenderer setCurrentTime:] in libopenFrameworks.a(ofAVFMovieRenderer.o)
  "_kCMTimeZero", referenced from:
      ___28-[AVFMovieRenderer loadURL:]_block_invoke_2 in libopenFrameworks.a(ofAVFMovieRenderer.o)
      -[AVFMovieRenderer stop] in libopenFrameworks.a(ofAVFMovieRenderer.o)
      -[AVFMovieRenderer setCurrentTime:] in libopenFrameworks.a(ofAVFMovieRenderer.o)
ld: symbol(s) not found for architecture i386
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[1]: *** [bin/videoPlayerExample] Error 1
make: *** [Release] Error 2

bakercp · 2015-01-12T19:10:48Z

I'm not doing any current makefile work, so if you want to make modifications go for it. I merged my latest work last week.

pizthewiz · 2015-01-12T22:38:02Z

Spun that separate make issue in PR #3553.

With OS X 10.8 as the minimum, the AV Foundation player can be used for all supported versions of OS X, 10.8-10.10.

With OS X 10.8 as the minimum, only the QTKit grabber is used though the QuickTime grabber seems to be used still as a fallback on Windows.

While the file was being compiled, the protection braces left the archive with very few symbols.

The high performance player example used ofQTKitPlayer directly and the settings there don't apply since it has been removed. In theory the AV Foundation player should perform far better anyways right? 😉

pizthewiz · 2015-01-13T00:12:00Z

I followed the the guide How to Rebase a Pull Request, to rebase all the commits in this PR to pick up the makefile fix in #3553.

arturoc · 2015-01-13T12:06:46Z

i don't think we should remove all this classes, even if it's not officially supported it's good that it's relatively easy to use OF with older versions.

i use to do workshops and there's several people who are still using versions as old as 10.6 and even if it's not supported doing a couple of changes in the project it works without problem. if we remove this classes it will be really difficult to use OF on older versions, right?

i would keep them around for a while even if they are not officially supported and have a quick way to change something in ofConstants that enables support for older versions, like have a commented ifdef so you just need to uncomment it to enable support for 10.7 or even 10.6

pizthewiz · 2015-01-13T20:35:17Z

i would keep them around for a while even if they are not officially supported and have a quick way to change something in ofConstants that enables support for older versions, like have a commented ifdef so you just need to uncomment it to enable support for 10.7 or even 10.6.

I worry this would lead to deferred tears of frustration due to code rot. If this code is hidden behind an #ifdef, it is even less likely that it'll be tested and known to compile, let alone known to function as expected.

In the particular case of ofQTKitPlayer, it has historically been problematic with several issues reported and closed as won't fix coming up to the 0.9.0 release (which I fully agree with).

GitHub Issues:

ofQTKitPlayer crash with ofxCocoaWindow #1778 ofQTKitPlayer crash with ofxCocoaWindow
HLS m3u8 HTTP live streaming video #1844 HLS m3u8 HTTP live streaming video
ofQTKitPlayer deadlock on delete #1916 ofQTKitPlayer deadlock on delete
ofVideoplayer getPosition getFrame #1928 ofVideoplayer getPosition getFrame
Bad access to ofQTKitPlayer Texture after a while. #2531 Bad access to ofQTKitPlayer Texture after a while.
Crash while playing internet videos with ofQTKitPlayer. #2655 Crash while playing internet videos with ofQTKitPlayer.

Forum Threads:

Is ofQTKitPlayer really worth preserving? People that need it, could they not use 0.8.4?

If we are just talking about 10.6 and 10.7, we could add an additional commented out section to ofConstants to opt them back into OF_VIDEO_PLAYER_QUICKTIME which has not been removed yet since it is a fallback for Windows, though it too likely receives minimal testing and is subject to code rot.

kylemcdonald · 2015-01-13T23:19:31Z

@arturoc i understand where you're coming from with wanting to make it possible to hack OF support together for older systems, but i think a commented #define is not an option because it will create more work for us (answering questions about enabling it) and confusion for users who are surprised it doesn't work out of the box.

instead, if we want to provide support for 10.6/7, we should support ofQTKitPlayer completely, re-open up the issues above, and milestone them for 0.9.0. then we need to either detect the operating system version and switch the video player appropriately (i think this is possible...?) or provide two separate download links, one for OS X 10.6/7 and one for 10.8+

given that we have limited time and energy as a team, and no one has shown interest in fixing the ofQTKitPlayer issues, i think @pizthewiz is correct in suggesting complete removal instead of half-support.

to get a better idea about how many 10.6/7 users there are, i checked google analytics. in the last three months, less than 5% of the openframeworks.cc visitors using OS X are using 10.6/7

arturoc · 2015-01-14T12:17:36Z

if it's a big problem go for it, i just think it's a pity to discard code that it's perfectly functional if all we need is a comment in OF constants to be able to run OF in older machines. almost every time i do a workshop there's someone who has a really old version and updating their OS is usually not an option so even if it's something slightly hacky it's good that it's there if it can be done without disrupting the official release

ofTheo · 2015-01-14T12:24:59Z

I guess I would echo what @arturoc is saying.

I think deprecating the ofQTKitVideoPlayer and making some big warnings come up - making it clear that its legacy code and will not be updated could work well to show that it won't receive additional support.

That would be my preference.

ofTheo · 2015-01-14T12:29:18Z

To add to that - I would say we should keep the QT player and the QTKit player for the versions of OS X that they can run on. No reason to break something that is working.

If other things don't work for 10.6 or 10.7 without some fiddling thats fine.

i.e.: we shouldn't put a lot of effort to get 10.6 or 10.7 working with OF master, but at the same time we shouldn't be actively trying to make it unsupported. :)

ofZach · 2015-01-14T12:32:11Z

have we deprecated ofQTKitVideoPlayer ? I think it makes sense to do that first as theo suggests and then remove it down the line. I personally don't mind being a little slow-er in terms of cutting things off.

I do have some larger questions about OSX and how we might need to bifurcate the releases. the xcode incompatibilities (esp ios simulator not working on xcode 6+) are quite challenging to folks and I can imagine that soon we need to have two releases.

bilderbuchi · 2015-01-14T12:32:43Z

I'd caution against a #define for switching this in the OF source, as then we would have to always pay attention if community-contributed PRs have that define set because the author is on an old machine. maybe a better approach would be to make this settable with a static function, or an environment variable that any legacy users can set themselves?
also, +1 on deprecation - the legacy users will just have to live with the warnings on compilation.

pizthewiz · 2015-01-14T17:34:47Z

Sorry if this PR was over-reaching, it does two things that are being semi-conflated, perhaps due to the PR title:

Raise the deployment target to 10.8
Remove the ofQTKitPlayer class

To fully remove and handling for < 10.8, we'd have to go through and remove a bunch of (would-be if this were merged) dead code and conditional handing that is no longer conditional. While I was mostly following @bakercp's checklist in #3507, I chose to remove ofQTKitPlayer instead of #if define( or comment it out for two reasons:

ofQTKitPlayer has been problematic and needs further development. We closed out a bunch of issues in the 0.9.0 build-up and I think if we are going to allow ofQTKitPlayer to be used, we should support it.
Even if we hid it away, (I personally think presence in the repo semi-communicates a supported status) it would only further aggravate the existing development needs of ofQTKitPlayer.

if it's a big problem go for it, i just think it's a pity to discard code that it's perfectly functional

My feeling is that ofQTKitPlayer is not perfectly functional, that we would be removing something that needs further development and support still.

@ofTheo

I think deprecating the ofQTKitVideoPlayer and making some big warnings come up

@ofZach

have we deprecated ofQTKitVideoPlayer ?

The ofQTKitPlayer class has not been deprecated in an OF release (we haven't had a more modern player for OS X until now), but all compilation on OS X 10.9+ has been met with QTKit deprecation warnings which became so numerous that @admsyn coalesced them in #2966 / cc66863 starting with OF-0.8.2.

If this proves to be too controversial, this PR could be closed and the OS minimum raised without any removal of ofQTKitPlayer.

pizthewiz · 2015-01-21T01:16:49Z

Is this PR 💀?

I would also mention that (unofficial) support for 10.6 and 10.7 will have implications for our tooling requirements and in turn also have an impact on our Objective-C feature use.

Should the deployment target OS change be split out?

pizthewiz · 2015-01-21T01:17:10Z

And what is Travis's damage?

kylemcdonald · 2015-01-21T01:37:56Z

@pizthewiz i'm not sure, it's hard to tell. it seems like there is pressure against it, but nobody has closed it, or restarted discussion on the original issues, or posted code for an alternative solution.

my suggestion is that we give it one more week (it's been one week since the last few comments) and anyone who disagrees about merging it is free to close the PR (hopefully, make a suggestion of what to do next). or if no one closes it in a week from now, we merge anyway. we stop supporting 10.6 and 10.7, but we can only support things to the extent that we have people to help support them.

(and ignore the travis status on a few of the issues, it doesn't mean anything yet)

ofZach · 2015-01-21T01:52:53Z

I suggest we get proper deprecation in (as Theo suggests), and after a short period with that in, we move on doing this. I think it's good to remove old code but also be thoughtful / vocal about deprecation.

sheridanis · 2015-01-21T09:04:10Z

Please don't completely remove qtkitplayer until avfoundation properly supports codecs, namely Hap. I know there are a heap of projects out there that need this for playback of large video files.

Sent from my toaster

On 21 Jan 2015, at 01:52, ofZach [email protected] wrote:

I suggest we get proper deprecation in (as Theo suggests), and after a short period with that in, we move on doing this. I think it's good to remove old code but also be thoughtful / vocal about deprecation.

—
Reply to this email directly or view it on GitHub.

pizthewiz · 2015-01-21T09:08:45Z

@sheridanis See the Hap Codecs now working in 64-bit AVFoundation blog post and the Vidvox/hap-in-avfoundation project. Though, those heap of projects could continue to use 0.8.4 as well 😉.

sheridanis · 2015-01-21T12:50:25Z

Awesome thanks for that I missed it with Christmas and all

On 21/01/2015 9:08 am, pizthewiz wrote:

@sheridanis https://github.com/sheridanis See the Hap Codecs now
working in 64-bit AVFoundation
http://vdmx.vidvox.net/blog/hap-in-64-bit-avfoundation blog post and
the Vidvox/hap-in-avfoundation
https://github.com/Vidvox/hap-in-avfoundation project. Though, those
heap of projects could continue to use 0.8.4 as well 😉.

—
Reply to this email directly or view it on GitHub
#3543 (comment).

kylemcdonald · 2015-02-08T19:48:31Z

closing this in favor of a clarification from the IRC meeting:

0.9.0 will not support 10.6 due to 64-bit issues.
0.9.0 will support 10.7 with ofQTKitPlayer but it will be deprecated.
0.9.0 on 10.8+ will use AVFoundation.

pizthewiz changed the title ~~Set OS X 10.8 as minimum~~ Remove 10.6 and 10.7, set 10.8 as the minimum Jan 12, 2015

pizthewiz mentioned this pull request Jan 12, 2015

Add missing frameworks to OS X make machinery #3553

Merged

Jean-Pierre Mouilleseaux added 8 commits January 12, 2015 16:08

remove video player handling for 10.6 and 10.7

ee93595

With OS X 10.8 as the minimum, the AV Foundation player can be used for all supported versions of OS X, 10.8-10.10.

remove the QTKit video player

f107128

add braces around AV Foundation player

b71a701

simplify capture assignment on OS X

6b5d358

With OS X 10.8 as the minimum, only the QTKit grabber is used though the QuickTime grabber seems to be used still as a fallback on Windows.

remove empty SDKROOT assignment

bb197d3

set OS X 10.8 as the minimum version

96d5737

remove braces that provded problematic

97ffed6

While the file was being compiled, the protection braces left the archive with very few symbols.

remove the high performance video player

ad1fead

The high performance player example used ofQTKitPlayer directly and the settings there don't apply since it has been removed. In theory the AV Foundation player should perform far better anyways right? 😉

pizthewiz force-pushed the felicide branch from 94a6c98 to ad1fead Compare January 13, 2015 00:10

ofTheo mentioned this pull request Jan 14, 2015

Unify OS X and iOS AV Foundation Player #3242

Closed

pizthewiz changed the title ~~Remove 10.6 and 10.7, set 10.8 as the minimum~~ Remove ofQTKitPlayer, set OS X 10.8 as the minimum Jan 20, 2015

This was referenced Feb 4, 2015

Remove Support for OSX 10.6 #3507

Closed

oF 64 bit, c++11 support (only merge once os x apothecary build binaries are in master) #3627

Merged

kylemcdonald closed this Feb 8, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove ofQTKitPlayer, set OS X 10.8 as the minimum #3543

Remove ofQTKitPlayer, set OS X 10.8 as the minimum #3543

pizthewiz commented Jan 11, 2015

pizthewiz commented Jan 12, 2015

pizthewiz commented Jan 12, 2015

bakercp commented Jan 12, 2015

pizthewiz commented Jan 12, 2015

pizthewiz commented Jan 13, 2015

arturoc commented Jan 13, 2015

pizthewiz commented Jan 13, 2015

kylemcdonald commented Jan 13, 2015

arturoc commented Jan 14, 2015

ofTheo commented Jan 14, 2015

ofTheo commented Jan 14, 2015

ofZach commented Jan 14, 2015

bilderbuchi commented Jan 14, 2015

pizthewiz commented Jan 14, 2015

pizthewiz commented Jan 21, 2015

pizthewiz commented Jan 21, 2015

kylemcdonald commented Jan 21, 2015

ofZach commented Jan 21, 2015

sheridanis commented Jan 21, 2015

pizthewiz commented Jan 21, 2015

sheridanis commented Jan 21, 2015

kylemcdonald commented Feb 8, 2015

Remove ofQTKitPlayer, set OS X 10.8 as the minimum #3543

Remove ofQTKitPlayer, set OS X 10.8 as the minimum #3543

Conversation

pizthewiz commented Jan 11, 2015

pizthewiz commented Jan 12, 2015

Testing Coverage

pizthewiz commented Jan 12, 2015

bakercp commented Jan 12, 2015

pizthewiz commented Jan 12, 2015

pizthewiz commented Jan 13, 2015

arturoc commented Jan 13, 2015

pizthewiz commented Jan 13, 2015

kylemcdonald commented Jan 13, 2015

arturoc commented Jan 14, 2015

ofTheo commented Jan 14, 2015

ofTheo commented Jan 14, 2015

ofZach commented Jan 14, 2015

bilderbuchi commented Jan 14, 2015

pizthewiz commented Jan 14, 2015

pizthewiz commented Jan 21, 2015

pizthewiz commented Jan 21, 2015

kylemcdonald commented Jan 21, 2015

ofZach commented Jan 21, 2015

sheridanis commented Jan 21, 2015

pizthewiz commented Jan 21, 2015

sheridanis commented Jan 21, 2015

kylemcdonald commented Feb 8, 2015