This is Xamarin's bug tracking system. For product support, please use the support links listed in your Xamarin Account.
Bug 1062 - CaptureStillImageAsynchronously uses wrong semantics for passed buffer
Summary: CaptureStillImageAsynchronously uses wrong semantics for passed buffer
Status: RESOLVED FIXED
Alias: None
Product: iOS
Classification: Xamarin
Component: Xamarin.iOS / monotouch (show other bugs)
Version: 1.0
Hardware: PC Mac OS
: --- normal
Target Milestone: Untriaged
Assignee: Bugzilla
URL:
: 3133 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-09-26 18:32 UTC by Miguel de Icaza
Modified: 2012-02-01 18:55 UTC (History)
5 users (show)

See Also:
Tags:


Attachments
The modified sample to take still pictures. (12.83 KB, application/zip)
2011-09-26 18:33 UTC, Miguel de Icaza
Details
proposed patch (650 bytes, patch)
2012-01-31 17:31 UTC, Rolf Bjarne Kvinge
Details | Diff
test project that QA can use to verify fix (device only): screen turns green if test passes (4.34 KB, application/zip)
2012-02-01 05:32 UTC, Rolf Bjarne Kvinge
Details

Description Miguel de Icaza 2011-09-26 18:32:43 UTC
In the attached sample, the method FinishedCapturingImage is called with a CMSampleBuffer without the proper ownership.

We assume that we own the buffer, but we do not, so two things happen:

Calling Dispose on the CMSampleBuffer means that we release it, and then Objective-C then releases it again, so we crash.   If we do not Dispose it, and let the GC finalize it, then Objective-C releases it, and then we call release again, crashing as well.

What we might need is to flag this method as us needing to take a reference for the duration of the call.    So we do need to flag the method in some form so our binding takes a ref.

The fix to the attached sample, you would need to do this:


CFRetain (sampleBuffer.Handle);

Before the call to sampleBuffer.Dispose.   The call to dispose needs to be there, to make sure than Handle is wiped, or the finalizer will happily do it later.
Comment 1 Miguel de Icaza 2011-09-26 18:33:33 UTC
Created attachment 500 [details]
The modified sample to take still pictures.

See support case #2334
Comment 2 Rolf Bjarne Kvinge 2012-01-31 09:14:22 UTC
*** Bug 3133 has been marked as a duplicate of this bug. ***
Comment 3 Daniel 2012-01-31 13:24:59 UTC
CFRetain as CFType's CFRetain method? Does it exist?

I'm sorry, but I don't understand what to do.
Comment 4 Sebastien Pouliot 2012-01-31 14:27:36 UTC
Daniel, CFRetain is not exposed in our public API. Until this is fixed you can pinvoke it manually using this definition.

		[DllImport ("/System/Library/Frameworks/CoreFoundation.framework/CoreFoundation")]
		extern static void CFRetain (IntPtr handle);
Comment 5 Rolf Bjarne Kvinge 2012-01-31 17:31:22 UTC
Created attachment 1296 [details]
proposed patch

Miguel, this patch fixes the problem. IMHO CMSampleBuffer parameters to managed delegates (callbacks) should always have this type of semantics (in any case there is only one place in our code we have a managed delegate with a CMSampleBuffer).
Comment 6 Miguel de Icaza 2012-01-31 20:28:38 UTC
This sounds about right, but it seems like iOS is itself inconsistent in this case.

Look at a similar use case in AVCaptureFrames/Main.cs, this is a video recording sample and without the call to Dispose () we would leak.   In this particular case, AVCaptureFrames makes it very obvious that the leak is present by stopping working after 13 frames shot which is the point where we exhaust the internal AVFoundation pool.

I suspect that we need to add a special attribute to flag cases where the ownership is different.
Comment 7 Miguel de Icaza 2012-01-31 20:45:09 UTC
I take my last comment back.

Rolf's comment was about invoking methods that take a function pointer parameter and one of their parameters is a CMSampleBuffer (an Objective-C block), while my comment applied to Objective-C delegate/callback patterns.

The patch looks good to go.
Comment 8 Rolf Bjarne Kvinge 2012-02-01 04:11:16 UTC
Patch committed: 2bb631f.
Comment 9 Rolf Bjarne Kvinge 2012-02-01 04:14:33 UTC
This fix will be included in any future 5.3.* releases.

Daniel: note that the proposed workaround (using CFRetain) will cause a leak now, so you should remove the workaround once you upgrade to a MonoTouch version which has this fix.
Comment 10 Rolf Bjarne Kvinge 2012-02-01 05:32:27 UTC
Created attachment 1299 [details]
test project that QA can use to verify fix (device only): screen turns green if test passes
Comment 11 PJ 2012-02-01 12:35:45 UTC
I have added the test case to testrails (for testing after 5.3 is released).

Just to be clear - is it an iPad only issue? (the test case solution is an iPad application)
Comment 12 Rolf Bjarne Kvinge 2012-02-01 18:55:33 UTC
PJ: it's not an iPad only issue, feel free to change the project to an iPhone project. However the project should only be run on device (since the simulator doesn't have a camera to take pictures with).

Note You need to log in before you can comment on or make changes to this bug.