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
: CaptureStillImageAsynchronously uses wrong semantics for passed buffer
Status: RESOLVED FIXED
Product: iOS
Classification: Xamarin
Component: Class Libraries
: 1.0
: PC Mac OS
: --- normal
: Untriaged
Assigned To: Bugzilla
:
:
:
:
  Show dependency treegraph
 
Reported: 2011-09-26 18:32 EDT by Miguel de Icaza
Modified: 2012-02-01 18:55 EST (History)
5 users (show)

See Also:
Tags:
Test Case URL: https://testrail.xamarin.com/index.php?/cases/view/3340
External Submit: ---


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

Description Miguel de Icaza 2011-09-26 18:32:43 EDT
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 EDT
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 EST
*** Bug 3133 has been marked as a duplicate of this bug. ***
Comment 3 Daniel 2012-01-31 13:24:59 EST
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 EST
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 EST
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 EST
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 EST
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 EST
Patch committed: 2bb631f.
Comment 9 Rolf Bjarne Kvinge 2012-02-01 04:14:33 EST
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 EST
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 EST
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 EST
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.