GNOME Bugzilla – Bug 704340
osxvideosink: code refactoring: splitting obj-c code to separate files
Last modified: 2018-05-04 09:01:49 UTC
I finally finished merging my changes with refactoring of osxvideosink code with some fixes. Here's a patch set on that. I hope, someone (perhaps, Andoni or Sebastian) would kindly review it and push it, if it's worth it. Here's some list of changes: - Objective C code of GstOSXVideoSinkObject, GstWindowDelegate, GstBufferObject and NSApplication was moved to separate set of files, nssinkobject.h(.m). So was all Cocoa threading-aware code. Now it's about 600 lines remaining in osxvideosink.m and another ~400 lines in nssinkobject.m. I hope, now it's really clearer and easier to read, analyze and debug - all the AutoReleasePool calls were eliminated as they were used in a little bit messy places and turned out to be quite unnecessary. I analyzed carefully all the places where memory for NS objects and Gst objects is allocated, there're not so many of them, so it's quite handy and efficient to control it manually - compilation and running in case of RUN_NS_APP_THREAD were restored and tested - setview was fixed to support changing window-id even in PLAYING state without any drawbacks, though it's not necessary - more full substitutions in GstOSXVideoSinkObject in case of RUN_NS_APP_THREAD defined to allow our NSThread pretend to be mainThread more consistent way (performSelectorOnMainThread now works out of the box) - some threading-related minor fixes In general, now all the GstOSXVideoSinkObject is called in main thread and do some complex run-loop logic while all the functions in osxvideosink do GStreamer-related things. I've tested the code using gst-launch-1.0 both with RUN_NS_APP_THREAD definition on and off, with single and multiple instances. So was performed with my Qt4 application to test GstVideoOverlay stuff (RUN_NS_APP_THREAD on and off, single and multiple instances). All the tests appear to finish successfully.
Created attachment 249297 [details] [review] Moving window and view data to GstOSXWindow
Created attachment 249298 [details] [review] GstGLView is created in separate function
Created attachment 249299 [details] [review] OSWindow's 'closed' flag is removed, several fixes
Created attachment 249300 [details] [review] gst_osx_video_sink_osxwindow_create() was changed
Created attachment 249301 [details] [review] Several changes in setView allowing to change window-id on the fly
Created attachment 249302 [details] [review] Most Objective-C and threading code moved to nssinkobject.h(.m)
Review of attachment 249297 [details] [review]: Basically looks good but: ::: sys/osxvideo/osxvideosink.m @@ +847,2 @@ } if (osxvideosink->osxwindow != NULL && view != NULL) { Is it guaranteed here that we always have ->osxwindow? Otherwise you'll lose the superview setting as you don't store it anywhere else. @@ +860,2 @@ if (osxvideosink->osxwindow) { + [osxvideosink->osxwindow->gstview addToSuperview: osxvideosink->osxwindow->superview]; Also this code does not check for osxwindow!=NULL, while the above does
Review of attachment 249300 [details] [review]: ::: sys/osxvideo/osxvideosink.m @@ +844,1 @@ + [osxwindow->internalwindow setDelegate:nil]; Are we guaranteed to always have an internalwindow here?
Comment on attachment 249301 [details] [review] Several changes in setView allowing to change window-id on the fly Note that changing the window on the fly is not really a supported behaviour by GstVideoOverlay. Some sinks implement it but it's not guaranteed to work with all.
Comment on attachment 249302 [details] [review] Most Objective-C and threading code moved to nssinkobject.h(.m) Not really reviewing this one here, would be great to split it into multiple patches. One just moving the code around (and thus not needing a real review) and others for the functional changes.
Thanks for all the work on this, but independent of that please note that currently it seems like a better option to switch to the sink in gst-plugins-gl for 1.3/1.4 (to keep all the GL related code in a single place and allow better GL integration). For 1.2 at least osxvideosink will still be used though.
(In reply to comment #7) Thanks, Sebastian. 1) It is guaranteed, that osxwindow exists in case set_window_handle() was called in states higher than READY. Is it generally expected to be called in any state? If so, that should be changed. Didn't take it into account, sorry. 2) I think, I don't get it. Do you mean, we should also check for superview here? Or should it be moved to previous branch?
(In reply to comment #8) Not really, but afaik it's basically safe to send messages to NULL objects.
(In reply to comment #9) > (From update of attachment 249301 [details] [review]) > Note that changing the window on the fly is not really a supported behaviour by > GstVideoOverlay. Some sinks implement it but it's not guaranteed to work with > all. Yes, I saw your clarification on this before. I just structured behaviour in setView() and on-the-fly functionality came for free, so it wasn't the main target.
(In reply to comment #10) > (From update of attachment 249302 [details] [review]) > Not really reviewing this one here, would be great to split it into multiple > patches. One just moving the code around (and thus not needing a real review) > and others for the functional changes. Yes, it's quite big now. The problem is that the splitted part was tightly connected to main part, so I'm afraid, there can't be a simple patch which just moves it to separate files. It's possible to reach, but *.m files would use parts of one another (with 'extern'). Sebastian, could you please suggest how can I split a commit in git, given that all these patches is committed in my local git repo? Never did this kind of things so it looks quite complex task for me.
(In reply to comment #11) > Thanks for all the work on this, but independent of that please note that > currently it seems like a better option to switch to the sink in gst-plugins-gl > for 1.3/1.4 (to keep all the GL related code in a single place and allow better > GL integration). For 1.2 at least osxvideosink will still be used though. Thanks for information, Sebastian, didn't know it. So, is this right, that gst-plugins-gl will be back and will work on all the GStreamer platforms?
(In reply to comment #12) > (In reply to comment #7) > Thanks, Sebastian. > > 1) It is guaranteed, that osxwindow exists in case set_window_handle() was > called in states higher than READY. Is it generally expected to be called in > any state? If so, that should be changed. Didn't take it into account, sorry. Yes > 2) I think, I don't get it. Do you mean, we should also check for superview > here? Or should it be moved to previous branch? No I mean that this code is using ->osxwindow unconditionally, while a few lines above you check if it's not NULL. So could crash probably. (In reply to comment #15) > (In reply to comment #10) > > (From update of attachment 249302 [details] [review] [details]) > > Not really reviewing this one here, would be great to split it into multiple > > patches. One just moving the code around (and thus not needing a real review) > > and others for the functional changes. > > Yes, it's quite big now. The problem is that the splitted part was tightly > connected to main part, so I'm afraid, there can't be a simple patch which just > moves it to separate files. It's possible to reach, but *.m files would use > parts of one another (with 'extern'). > > Sebastian, could you please suggest how can I split a commit in git, given that > all these patches is committed in my local git repo? Never did this kind of > things so it looks quite complex task for me. Ok, leave it as is then :) It looks good, but I might've missed something. So would prefer to have Andoni look at it too. (In reply to comment #16) > (In reply to comment #11) > > Thanks for all the work on this, but independent of that please note that > > currently it seems like a better option to switch to the sink in gst-plugins-gl > > for 1.3/1.4 (to keep all the GL related code in a single place and allow better > > GL integration). For 1.2 at least osxvideosink will still be used though. > > Thanks for information, Sebastian, didn't know it. So, is this right, that > gst-plugins-gl will be back and will work on all the GStreamer platforms? That's the plan, and it should already work on X11, Wayland, OSX, Windows and Android right now. X11, Wayland and Windows will use other, more optimal sinks by default though (there's better API then GL available on these platforms).
(In reply to comment #17) > (In reply to comment #12) > > (In reply to comment #7) > > 1) It is guaranteed, that osxwindow exists in case set_window_handle() was > > called in states higher than READY. Is it generally expected to be called in > > any state? If so, that should be changed. Didn't take it into account, sorry. > > Yes OK. Would it be normal to fix it on top of all these changes? I think, it would be some minor changes. > > 2) I think, I don't get it. Do you mean, we should also check for superview > > here? Or should it be moved to previous branch? > > No I mean that this code is using ->osxwindow unconditionally, while a few > lines above you check if it's not NULL. So could crash probably. No, not really. This check: "if (osxvideosink->osxwindow) {" isn't removed so it should be safe to use ->osxwindow. > (In reply to comment #15) > Ok, leave it as is then :) It looks good, but I might've missed something. So > would prefer to have Andoni look at it too. Yes, thank you. Let's wait for Andoni's review. > (In reply to comment #16) > That's the plan, and it should already work on X11, Wayland, OSX, Windows and > Android right now. X11, Wayland and Windows will use other, more optimal sinks > by default though (there's better API then GL available on these platforms). Very nice. So positive news, hope to try it for OSX. Especially given that osxvideosink has quite serious problems with GL performance.
(In reply to comment #18) > (In reply to comment #17) > > (In reply to comment #12) > > > (In reply to comment #7) > > > 1) It is guaranteed, that osxwindow exists in case set_window_handle() was > > > called in states higher than READY. Is it generally expected to be called in > > > any state? If so, that should be changed. Didn't take it into account, sorry. > > > > Yes > > OK. Would it be normal to fix it on top of all these changes? I think, it would > be some minor changes. Yes, just do it on top > > (In reply to comment #16) > > That's the plan, and it should already work on X11, Wayland, OSX, Windows and > > Android right now. X11, Wayland and Windows will use other, more optimal sinks > > by default though (there's better API then GL available on these platforms). > > Very nice. So positive news, hope to try it for OSX. Especially given that > osxvideosink has quite serious problems with GL performance. Please do and report :) You need to use 1.1.2 or git master for that to work though.
> Yes, just do it on top Ok, thanks, will fix it. > Please do and report :) You need to use 1.1.2 or git master for that to work > though. Yes, trying to build it. And are there any plans to also merge egglessink to gst-plugins-gl? egglessink is very useful for Raspberry Pi, would be great to have all in one.
(In reply to comment #20) > Yes, trying to build it. And are there any plans to also merge egglessink to > gst-plugins-gl? egglessink is very useful for Raspberry Pi, would be great to > have all in one. Yes, should be trivial to add RPi support to gst-plugins-gl. It's on my list, bug #703342
It will be great. Thanks for information. Can't pay any time to RPi now, but hope to return to it some time in the future, output through gst-plugins-gl looks very promising.
Created attachment 249765 [details] [review] Remove osxwindow instance and preserve window handle Here's a patch with removing osxwindow instance and addressing an issue with window handle loss Sebastian mentioned in comment #7.
Review of attachment 249765 [details] [review]: In which order do all these patches have to be applied now? Can you put them into a git branch somewhere maybe, like on github?
I tried to make it be applied on top of previous patches mentioned here. I just tried it on git master, everything seem to work fine.
Ok, I'll push all this once Andoni took a look at the very large patch too :)
Yes, thank you.
Andoni?
Ping? :)
Andoni, ping?
Do we want to keep this open? I think the plan is to remove osxvideosink and replace it with glimagesink entirely.
Well, as to me, I think, it's quite outdated, too. Although, as I remember, there're some problems fixed, but there were also some remaining ones in OpenGL code which I didn't managed to fix.
Thanks for the interest, but this hasn't seen any proper activity in 5 years, and the current goal for OSX video output is to use the GL sinks.