GNOME Bugzilla – Bug 777847
ios/osx: Don't use manual reference counting in ObjC code
Last modified: 2017-03-07 11:38:14 UTC
It's apparently deprecated and the Internet[0] says that Apple does not allow it anymore starting from May 2015 (!). Moving to ARC should be relatively simple hopefully: https://developer.apple.com/library/content/releasenotes/ObjectiveC/RN-TransitioningToARC/Introduction/Introduction.html [0] http://www.iclarified.com/47249/developer.apple.com/library/ios/releasenotes/ObjectiveC/RN-TransitioningToARC/Introduction/Introduction.html
Created attachment 344859 [details] [review] Upgrade gst-plugins-pad/applemedia to ARC This is my first attempt at converting the code to ARC. It compiles in the context of my XCode project and it successfully make's inside gst-plugins-bad. However, the makefile in the latter needs to be updated to use ARC, which I don't know how to do. I have successfully tested iosassetsrc in my project, and it seems to be fine with these changes. However, I'm new to Objective-C programming and I'm not 100% confident that every change I have made is correct and wont lead to prematurely freeing objects or memory leaks.
Created attachment 344923 [details] [review] Upgrade gst-plugins-pad/applemedia to ARC
I have extensively tested this patch with avfvideosrc now and it works well.
This has gone through extensive stress testing now -- on iOS. It should be very easy to get OSX working as well. Can we move forward on this patch?
Review of attachment 344923 [details] [review]: Looks good to me, but misses the macOS bits and won't compile there as-is
Created attachment 347269 [details] [review] OS X code converted to ARC; gst-plugins-bad makes on MacOS I'm keep this patch separate so the code can be reviewed independently. I can merge them all if you like. There is one more patch to come.
(In reply to Nick Kallen from comment #6) > I'm keep this patch separate so the code can be reviewed independently. I > can merge them all if you like. Please keep them separate to make reviewing easier, but I'll merge them all together when integrating them (so that each commit stays compileable)
Created attachment 347270 [details] [review] Same as previous patch with a few whitespace adjustments
Created attachment 347276 [details] [review] EAGL and two applemedia warning issues
Created attachment 347277 [details] [review] Same as 347270, but works with master Attachment 347270 [details] merges with 10.4.0 but not with master. This attachment is practically identical but will merge cleanly with master.
Review of attachment 347276 [details] [review]: Generally looks good but: ::: gst-libs/gst/gl/eagl/gstglwindow_eagl.m @@ +164,2 @@ callback (data); + gst_object_unref (context); Wrong indentation ::: sys/applemedia/videotexturecache.m @@ +145,3 @@ CFRelease(data->texture); CFRelease(data->cache); + free(data); Why do you revert this? @@ +200,2 @@ success: { + TextureWrapper *texture_data = malloc(sizeof(TextureWrapper)); And this?
Review of attachment 347277 [details] [review]: Looks good please provide another patch for: ::: ext/gl/caopengllayersink.m @@ +330,3 @@ + + /* FIXME it appears that the layer is leaked in pre-ARC code; + * leaving its lifecycle unchanged for now. */ You can release it in READY->NULL in gst_ca_opengl_layer_sink_change_state(). You're right that it's leaked
> ::: sys/applemedia/videotexturecache.m > @@ +145,3 @@ > CFRelease(data->texture); > CFRelease(data->cache); > + free(data); > > Why do you revert this? > > @@ +200,2 @@ > success: { > + TextureWrapper *texture_data = malloc(sizeof(TextureWrapper)); > > And this? Sorry this is a merge conflict, will fix
I think you also need to add something to OBJCFLAGS in the Makefile.am for ARC, right? Or does it work by default nowadays?
The flag is -fobjc-arc and I've been using 'OBJC_ARGS=-fobjc-arc ./configure' to compile. There's no existing OBJC_ARGS flag in Makefile.am ... Should I just add it?
It should be libblablabla_la_OBJCFLAGS. See sys/applemedia/Makefile.am which already has libgstapplemedia_la_OBJCFLAGS. You need to ensure that $(GST_OBJCFLAGS) is also part of it.
Created attachment 347315 [details] [review] Addressing code review feedback; 3 bug fixes in OSX ARC code
Created attachment 347317 [details] [review] Code review issues and bug fixes
Created attachment 347380 [details] [review] Makefile changes
commit 46bbc60d24c1914380b4145fa8595aa2e958c7dc Author: Nick Kallen <nickkallen@me.com> Date: Fri Feb 3 14:46:39 2017 +0100 applemedia/gl: Update code to use ARC All code interacting with Objective-C objects should now use Automated Reference Counting rather than manual memory management or Garbage Collection. Because ARC prohibits C-structs from containing references to Objective-C objects, all such fields are now typed 'gpointer'. Setting and gettings Objective-C fields on such a struct now uses explicit __bridge_* calls to tell ARC about object lifetimes. https://bugzilla.gnome.org/show_bug.cgi?id=777847