CRASH in CVPixelBufferGetBytePointerCallback()

• Component: New Bugs
• Status: RESOLVED FIXED

[Jer Noble]

CRASH in CVPixelBufferGetBytePointerCallback()

[Jer Noble]

<rdar://43824620>

[Radar WebKit Bug Importer]

<rdar://problem/44875084>

[Jer Noble]

Created attachment 351114 [details]
Patch

[Eric Carlson]

Comment on attachment 351114 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=351114&action=review

> Source/WebCore/platform/graphics/cv/PixelBufferConformerCV.cpp:108
> +    --info->lockCount;

Might be worth adding something like "ASSERT(info->lockCount > 0)"

[Tim Horton]

Comment on attachment 351114 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=351114&action=review

> Source/WebCore/platform/graphics/cv/PixelBufferConformerCV.cpp:128
> +            CVReturn result = CVPixelBufferLockBaseAddress(info->pixelBuffer.get(), kCVPixelBufferLock_ReadOnly);

I think you want an unlock here.

[Jer Noble]

Created attachment 351115 [details]
Patch

Removed the Lock from the info struct, fixed the incorrect lock in the release info callback, and added release logging of the CVPixelBuffer size in the get byte pointer callback.

[Jer Noble]

<rdar://problem/43824620>

[Jer Noble]

Comment on attachment 351114 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=351114&action=review

>> Source/WebCore/platform/graphics/cv/PixelBufferConformerCV.cpp:108
>> +    --info->lockCount;
> 
> Might be worth adding something like "ASSERT(info->lockCount > 0)"

Good idea.

[Jer Noble]

Created attachment 351138 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 351138 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=351138&action=review

> Source/WebCore/platform/graphics/cv/PixelBufferConformerCV.cpp:61
> +    int lockCount { 0 };

Since you changed the pointer type to be RetainPtr<>, can't we get rid of lockCount and use

CFRetain(pixelBuffer.get())
CFRelease(pixelBuffer.get())
CFGetRetainCount(pixelBuffer.get())

Instead of:
++info->lockCount
--info->lockCount
info->lockCount

I think it is conceptually correct as well. You can consider the lock count as a ref/retain count.

> Source/WebCore/platform/graphics/cv/PixelBufferConformerCV.cpp:83
> +    ++info->lockCount;

Is this code thread-safe?

> Source/WebCore/platform/graphics/cv/PixelBufferConformerCV.cpp:85
> +    RELEASE_LOG_INFO(Media, "CVPixelBufferGetBytePointerCallback() returning bytePointer: %p, size: %zu", address, CVPixelBufferGetDataSize(info->pixelBuffer.get()));

How often does this function get called? If it's called many times, I would suggest removing this particular RELEASE_LOG_INFO statement because it might add tons of unneeded information.

> Source/WebCore/platform/graphics/cv/PixelBufferConformerCV.cpp:139
> +    info->pixelBuffer = nullptr;
> +    delete info;

If you change info to std::unique_ptr or you pass RetainPtr<CVPixelBufferRef>, you will not have to do any of these releases. The pointer will be delete itself once the caller of CVPixelBufferReleaseInfoCallback() releases the last reference to this pointer.

> Source/WebCore/platform/graphics/cv/PixelBufferConformerCV.cpp:180
> +    CVPixelBufferInfo* info = new CVPixelBufferInfo();

We usually do not do bare "new" calls. 

Can't we do it this way?
    CVPixelBufferInfo* info = std::make_unique<CVPixelBufferInfo>().release();

Or can't we make info a std::unique_ptr?
    auto info = std::make_unique<CVPixelBufferInfo>();

Or can't we pass the RetainPtr itself instead of the "info" pointer and use CFRetain/CFRelease instead of lockCount?
    RetainPtr<CVPixelBufferRef> pixelBuffer = WTFMove(buffer);

[Jer Noble]

(In reply to Said Abou-Hallawa from comment #10)
> Comment on attachment 351138 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=351138&action=review
> 
> > Source/WebCore/platform/graphics/cv/PixelBufferConformerCV.cpp:61
> > +    int lockCount { 0 };
> 
> Since you changed the pointer type to be RetainPtr<>, can't we get rid of
> lockCount and use
> 
> CFRetain(pixelBuffer.get())
> CFRelease(pixelBuffer.get())
> CFGetRetainCount(pixelBuffer.get())
> 
> Instead of:
> ++info->lockCount
> --info->lockCount
> info->lockCount

No, because that would turn a logging change into a behavior change. We don't want to map locking the pixel buffer onto ref'ing the buffer itself, especially if the client isn't matching up its locks and unlocks.

> I think it is conceptually correct as well. You can consider the lock count
> as a ref/retain count.

No, you absolutely cannot. The lock count is on the CVPixelBuffer's internal storage, not the CVPixelBuffer itself.  Their lifetimes are not the same, and we can't treat them as the same.

> > Source/WebCore/platform/graphics/cv/PixelBufferConformerCV.cpp:83
> > +    ++info->lockCount;
> 
> Is this code thread-safe?

Nope!  But again, this is just a logging change.  If the caller is calling lock and unlock simultaneously on different threads, there's already huge problems because the pointer returned by lock() won't be valid after unlock() is called.+

> > Source/WebCore/platform/graphics/cv/PixelBufferConformerCV.cpp:85
> > +    RELEASE_LOG_INFO(Media, "CVPixelBufferGetBytePointerCallback() returning bytePointer: %p, size: %zu", address, CVPixelBufferGetDataSize(info->pixelBuffer.get()));
> 
> How often does this function get called? If it's called many times, I would
> suggest removing this particular RELEASE_LOG_INFO statement because it might
> add tons of unneeded information.

According to my testing, this is called twice per enter- or exit-fullscreen operation, which is not very often. The entire purpose of this logging is to allow us to co-relate the crash location to what CV thinks the size of the buffer is, so I think this is particularly essential logging.

> > Source/WebCore/platform/graphics/cv/PixelBufferConformerCV.cpp:139
> > +    info->pixelBuffer = nullptr;
> > +    delete info;
> 
> If you change info to std::unique_ptr or you pass
> RetainPtr<CVPixelBufferRef>, you will not have to do any of these releases.
> The pointer will be delete itself once the caller of
> CVPixelBufferReleaseInfoCallback() releases the last reference to this
> pointer.

Not sure I really understand your point here. The CVPixelBuffer is stored in a RetainPtr.  And info can't be stored in a std::unique_ptr, since it's just passed around as a void*.

> > Source/WebCore/platform/graphics/cv/PixelBufferConformerCV.cpp:180
> > +    CVPixelBufferInfo* info = new CVPixelBufferInfo();
> 
> We usually do not do bare "new" calls. 
> 
> Can't we do it this way?
>     CVPixelBufferInfo* info =
> std::make_unique<CVPixelBufferInfo>().release();

How does that solve anything? That's just a complicated wrapper around 'new'.

> Or can't we make info a std::unique_ptr?
>     auto info = std::make_unique<CVPixelBufferInfo>();
> 
> Or can't we pass the RetainPtr itself instead of the "info" pointer and use
> CFRetain/CFRelease instead of lockCount?
>     RetainPtr<CVPixelBufferRef> pixelBuffer = WTFMove(buffer);

No.

Using a NSObject* instead of a C++ object is just kicking the can down the road. It doesn't solve anything on its own. This data structure still needs to be passed around as a void* and cast to a type.

[Jer Noble]

Created attachment 351263 [details]
Patch

One more fix for the bots.

[WebKit Commit Bot]

Comment on attachment 351263 [details]
Patch

Clearing flags on attachment: 351263

Committed r236806: <https://trac.webkit.org/changeset/236806>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.