After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 777847 - ios/osx: Don't use manual reference counting in ObjC code
ios/osx: Don't use manual reference counting in ObjC code
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal normal
: 1.11.90
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 778333
 
 
Reported: 2017-01-27 16:27 UTC by Sebastian Dröge (slomo)
Modified: 2017-03-07 11:38 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Upgrade gst-plugins-pad/applemedia to ARC (35.88 KB, patch)
2017-02-03 13:59 UTC, Nick Kallen
none Details | Review
Upgrade gst-plugins-pad/applemedia to ARC (35.91 KB, patch)
2017-02-04 12:02 UTC, Nick Kallen
committed Details | Review
OS X code converted to ARC; gst-plugins-bad makes on MacOS (22.28 KB, patch)
2017-03-05 15:38 UTC, Nick Kallen
none Details | Review
Same as previous patch with a few whitespace adjustments (22.21 KB, patch)
2017-03-05 15:51 UTC, Nick Kallen
none Details | Review
EAGL and two applemedia warning issues (12.45 KB, patch)
2017-03-05 19:02 UTC, Nick Kallen
committed Details | Review
Same as 347270, but works with master (21.77 KB, patch)
2017-03-05 19:32 UTC, Nick Kallen
committed Details | Review
Addressing code review feedback; 3 bug fixes in OSX ARC code (3.71 KB, patch)
2017-03-06 14:03 UTC, Nick Kallen
none Details | Review
Code review issues and bug fixes (5.53 KB, patch)
2017-03-06 14:32 UTC, Nick Kallen
committed Details | Review
Makefile changes (1.68 KB, patch)
2017-03-07 10:41 UTC, Nick Kallen
committed Details | Review

Description Sebastian Dröge (slomo) 2017-01-27 16:27:05 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
Comment 1 Nick Kallen 2017-02-03 13:59:52 UTC
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.
Comment 2 Nick Kallen 2017-02-04 12:02:46 UTC
Created attachment 344923 [details] [review]
Upgrade gst-plugins-pad/applemedia to ARC
Comment 3 Nick Kallen 2017-02-07 10:40:52 UTC
I have extensively tested this patch with avfvideosrc now and it works well.
Comment 4 Nick Kallen 2017-02-26 09:43:28 UTC
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?
Comment 5 Sebastian Dröge (slomo) 2017-02-26 10:10:42 UTC
Review of attachment 344923 [details] [review]:

Looks good to me, but misses the macOS bits and won't compile there as-is
Comment 6 Nick Kallen 2017-03-05 15:38:00 UTC
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.
Comment 7 Sebastian Dröge (slomo) 2017-03-05 15:47:00 UTC
(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)
Comment 8 Nick Kallen 2017-03-05 15:51:16 UTC
Created attachment 347270 [details] [review]
Same as previous patch with a few whitespace adjustments
Comment 9 Nick Kallen 2017-03-05 19:02:30 UTC
Created attachment 347276 [details] [review]
EAGL and two applemedia warning issues
Comment 10 Nick Kallen 2017-03-05 19:32:40 UTC
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.
Comment 11 Sebastian Dröge (slomo) 2017-03-06 07:57:43 UTC
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?
Comment 12 Sebastian Dröge (slomo) 2017-03-06 08:01:45 UTC
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
Comment 13 Nick Kallen 2017-03-06 08:06:36 UTC
> ::: 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
Comment 14 Sebastian Dröge (slomo) 2017-03-06 08:07:39 UTC
I think you also need to add something to OBJCFLAGS in the Makefile.am for ARC, right? Or does it work by default nowadays?
Comment 15 Nick Kallen 2017-03-06 08:15:31 UTC
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?
Comment 16 Sebastian Dröge (slomo) 2017-03-06 08:21:21 UTC
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.
Comment 17 Nick Kallen 2017-03-06 14:03:40 UTC
Created attachment 347315 [details] [review]
Addressing code review feedback; 3 bug fixes in OSX ARC code
Comment 18 Nick Kallen 2017-03-06 14:32:33 UTC
Created attachment 347317 [details] [review]
Code review issues and bug fixes
Comment 19 Nick Kallen 2017-03-07 10:41:08 UTC
Created attachment 347380 [details] [review]
Makefile changes
Comment 20 Sebastian Dröge (slomo) 2017-03-07 11:37:31 UTC
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