Summary: | AcceleratedImageBuffer not instantiated but objects are punned to the type | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kimmo Kinnunen <kkinnunen> | ||||||||||||||||
Component: | Canvas | Assignee: | Kimmo Kinnunen <kkinnunen> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | cdumez, dino, sabouhallawa, simon.fraser, webkit-bug-importer | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Bug Depends on: | 226963 | ||||||||||||||||||
Bug Blocks: | 226962 | ||||||||||||||||||
Attachments: |
|
Description
Kimmo Kinnunen
2021-06-11 05:38:00 PDT
Created attachment 431188 [details]
Patch
Created attachment 431189 [details]
Patch
Created attachment 431190 [details]
Patch
Created attachment 431191 [details]
Patch
Created attachment 431192 [details]
Patch
Comment on attachment 431192 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431192&action=review > Source/WebCore/platform/graphics/PlatformImageBuffer.h:45 > +namespace Detail { I am not sure I agree with this change especially there are similar definitions in PlatformImageBufferShareableBackend.h. Can we move this change to a operate patch since it is unrelated to what you are trying to fix? > Source/WebCore/platform/graphics/PlatformImageBuffer.h:82 > + IOSurfaceImageBuffer(const WebCore::ImageBufferBackend::Parameters& parameters, std::unique_ptr<ImageBufferIOSurfaceBackend>&& backend) > + : Base(parameters, WTFMove(backend)) > + { > + } This constructor can be replaced by: using Base::Base; > Source/WebCore/platform/graphics/cg/ImageBufferCGBitmapBackend.h:40 > + WEBCORE_EXPORT static IntSize calculateSafeBackendSize(const Parameters&); > + WEBCORE_EXPORT static size_t calculateMemoryCost(const Parameters&); Why do we need to export these functions? I guess this is because the new api test calls ConcreteImageBuffer::create()? > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:280 > + auto outputImageBuffer = IOSurfaceImageBuffer::create(clampedSize, lastEffect.filter().filterScale(), lastEffect.resultColorSpace(), PixelFormat::BGRA8); What is the point of using this local variable and then moving it to the member m_outputImageBuffer? Is this because m_outputImageBuffer defined to be RefPtr<ImageBuffer> and you want to avoid casting? RefPtr<ImageBuffer> m_outputImageBuffer; I think it is better to change it to be RefPtr<AcceleratedImageBuffer> or even better RefPtr<IOSurfaceImageBuffer>. Created attachment 431306 [details]
Patch
Created attachment 431307 [details]
Patch for landing
(In reply to Said Abou-Hallawa from comment #6) > > Source/WebCore/platform/graphics/coreimage/FilterEffectRendererCoreImage.mm:280 > > + auto outputImageBuffer = IOSurfaceImageBuffer::create(clampedSize, lastEffect.filter().filterScale(), lastEffect.resultColorSpace(), PixelFormat::BGRA8); > > What is the point of using this local variable and then moving it to the > member m_outputImageBuffer? 1) The principle of members should not be assigned unless the function succeeds 2) The principle of big allocations should not be done before clearing up old allocations, in cases where the allocating function destroys the existing state in success and error case Moved these to bug 226962 Committed r278832 (238783@main): <https://commits.webkit.org/238783@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 431307 [details]. |