Bug 1062 - CaptureStillImageAsynchronously uses wrong semantics for passed buffer
Summary: CaptureStillImageAsynchronously uses wrong semantics for passed buffer
Alias: None
Product: iOS
Classification: Xamarin
Component: Xamarin.iOS.dll ()
Version: 1.0
Hardware: PC Mac OS
: --- normal
Target Milestone: Untriaged
Assignee: Bugzilla
: 3133 ()
Depends on:
Reported: 2011-09-26 18:32 UTC by Miguel de Icaza [MSFT]
Modified: 2012-02-01 18:55 UTC (History)
5 users (show)

Is this bug a regression?: ---
Last known good build:

The modified sample to take still pictures. (12.83 KB, application/zip)
2011-09-26 18:33 UTC, Miguel de Icaza [MSFT]
proposed patch (650 bytes, patch)
2012-01-31 17:31 UTC, Rolf Bjarne Kvinge [MSFT]
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 [MSFT]

Notice (2018-05-24): bugzilla.xamarin.com is now in read-only mode.

Please join us on Visual Studio Developer Community and in the Xamarin and Mono organizations on GitHub to continue tracking issues. Bugzilla will remain available for reference in read-only mode. We will continue to work on open Bugzilla bugs, copy them to the new locations as needed for follow-up, and add the new items under Related Links.

Our sincere thanks to everyone who has contributed on this bug tracker over the years. Thanks also for your understanding as we make these adjustments and improvements for the future.

Please create a new report on Developer Community or GitHub with your current version information, steps to reproduce, and relevant error messages or log files if you are hitting an issue that looks similar to this resolved bug and you do not yet see a matching new report.

Related Links:

Description Miguel de Icaza [MSFT] 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 [MSFT] 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 [MSFT] 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 [MSFT] 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 [MSFT] 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 [MSFT] 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 [MSFT] 2012-02-01 04:11:16 UTC
Patch committed: 2bb631f.
Comment 9 Rolf Bjarne Kvinge [MSFT] 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 [MSFT] 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 [MSFT] 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).