Summary: | Compress snapshots on iOS | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Beth Dakin <bdakin> | ||||||
Component: | WebKit2 | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | andersca, bdakin, commit-queue, simon.fraser, thorton, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | Other | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Beth Dakin
2015-10-05 11:52:23 PDT
Created attachment 262453 [details]
Patch
Attachment 262453 [details] did not pass style-queue:
ERROR: Source/WebCore/platform/graphics/cocoa/IOSurface.mm:273: Place brace on its own line for function definitions. [whitespace/braces] [4]
ERROR: Source/WebCore/platform/graphics/cocoa/IOSurface.h:40: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/cocoa/IOSurface.h:48: The parameter name "pixelFormat" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/spi/cocoa/IOSurfaceSPI.h:108: The parameter name "allocator" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/spi/cocoa/IOSurfaceSPI.h:109: The parameter name "accelerator" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/spi/cocoa/IOSurfaceSPI.h:114: Tab found; better to use spaces [whitespace/tab] [1]
ERROR: Source/WebCore/platform/spi/cocoa/IOSurfaceSPI.h:115: Tab found; better to use spaces [whitespace/tab] [1]
ERROR: Source/WebCore/platform/spi/cocoa/IOSurfaceSPI.h:116: Tab found; better to use spaces [whitespace/tab] [1]
ERROR: Source/WebCore/platform/spi/cocoa/IOSurfaceSPI.h:119: The parameter name "accelerator" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 9 in 7 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 262453 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262453&action=review > Source/WebCore/platform/graphics/cocoa/IOSurface.h:39 > +enum class PixelFormatForColor { I don't think we need the "ForColor" here, and I think maybe this should be inside WebCore::IOSurface? Maybe WebCore::IOSurface::Format? > Source/WebCore/platform/graphics/cocoa/IOSurface.h:42 > + YUVCompressed I don't think we need "compressed" here. I think this should be more specific, too: YUV422, or something like that. > Source/WebCore/platform/graphics/cocoa/IOSurface.h:91 > + WEBCORE_EXPORT void convertToFormat(PixelFormatForColor, std::function<void(std::unique_ptr<WebCore::IOSurface>)>); This should probably be a class method that takes a move-only WebCore::IOSurface, so that it's clear that it consumes the surface. Otherwise someone could accidentally keep using the now dead surface. Something like this: WEBCORE_EXPORT static void convertToFormat(std::unique_ptr<WebCore::IOSurface>&& inSurface, PixelFormatForColor, std::function<void(std::unique_ptr<WebCore::IOSurface>)>); > Source/WebCore/platform/graphics/cocoa/IOSurface.mm:264 > + static IOSurfaceAcceleratorRef accelerator; > + if (!accelerator) { > + IOSurfaceAcceleratorCreate(nullptr, nullptr, &accelerator); > + > + auto runLoopSource = IOSurfaceAcceleratorGetRunLoopSource(accelerator); > + CFRunLoopAddSource(CFRunLoopGetMain(), runLoopSource, kCFRunLoopDefaultMode); > + } We should check with the appropriate folks that it's OK to keep one of these alive forever. I'm not sure. > Source/WebCore/platform/graphics/cocoa/IOSurface.mm:266 > + auto destinationSurface = IOSurface::create(m_size, m_colorSpace, pixelFormat); We should bail (call the callback synchronously with the current surface) if the pixel formats are the same. > Source/WebCore/platform/graphics/cocoa/IOSurface.mm:274 > + no newline here > Source/WebCore/platform/graphics/cocoa/IOSurface.mm:282 > + IOSurfaceAcceleratorTransformSurface(accelerator, m_surface.get(), destinationIOSurfaceRef, nullptr, nullptr, &completion, nullptr, nullptr); Please retrieve the IOReturn from this and from IOSurfaceAcceleratorCreate and ASSERT_UNUSED(ret, ret == kIOReturnSuccess); like we do in other places. Created attachment 262458 [details]
Patch
(In reply to comment #4) > Comment on attachment 262453 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=262453&action=review > > > Source/WebCore/platform/graphics/cocoa/IOSurface.h:39 > > +enum class PixelFormatForColor { > > I don't think we need the "ForColor" here, and I think maybe this should be > inside WebCore::IOSurface? Maybe WebCore::IOSurface::Format? > Fixed! I went with your suggestion. > > Source/WebCore/platform/graphics/cocoa/IOSurface.h:42 > > + YUVCompressed > > I don't think we need "compressed" here. I think this should be more > specific, too: YUV422, or something like that. > Fixed! I again went with your suggestion. > > Source/WebCore/platform/graphics/cocoa/IOSurface.h:91 > > + WEBCORE_EXPORT void convertToFormat(PixelFormatForColor, std::function<void(std::unique_ptr<WebCore::IOSurface>)>); > > This should probably be a class method that takes a move-only > WebCore::IOSurface, so that it's clear that it consumes the surface. > Otherwise someone could accidentally keep using the now dead surface. > Something like this: > > WEBCORE_EXPORT static void > convertToFormat(std::unique_ptr<WebCore::IOSurface>&& inSurface, > PixelFormatForColor, > std::function<void(std::unique_ptr<WebCore::IOSurface>)>); > Fixed! Should I move it to the top of the file? It doesn't really matter, but I feel like that is where class methods go. > > Source/WebCore/platform/graphics/cocoa/IOSurface.mm:264 > > + static IOSurfaceAcceleratorRef accelerator; > > + if (!accelerator) { > > + IOSurfaceAcceleratorCreate(nullptr, nullptr, &accelerator); > > + > > + auto runLoopSource = IOSurfaceAcceleratorGetRunLoopSource(accelerator); > > + CFRunLoopAddSource(CFRunLoopGetMain(), runLoopSource, kCFRunLoopDefaultMode); > > + } > > We should check with the appropriate folks that it's OK to keep one of these > alive forever. I'm not sure. > Okay…Will follow up on this. > > Source/WebCore/platform/graphics/cocoa/IOSurface.mm:266 > > + auto destinationSurface = IOSurface::create(m_size, m_colorSpace, pixelFormat); > > We should bail (call the callback synchronously with the current surface) if > the pixel formats are the same. > > > Source/WebCore/platform/graphics/cocoa/IOSurface.mm:274 > > + > > no newline here > Fixed! > > Source/WebCore/platform/graphics/cocoa/IOSurface.mm:282 > > + IOSurfaceAcceleratorTransformSurface(accelerator, m_surface.get(), destinationIOSurfaceRef, nullptr, nullptr, &completion, nullptr, nullptr); > > Please retrieve the IOReturn from this and from IOSurfaceAcceleratorCreate > and ASSERT_UNUSED(ret, ret == kIOReturnSuccess); like we do in other places. Fixed! Thanks Tim! Comment on attachment 262458 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262458&action=review > Source/WebCore/ChangeLog:12 > + Though the default is still RGBA, it is now possible to create and IOSurface "and" > Source/WebCore/platform/graphics/cocoa/IOSurface.mm:259 > + auto runLoopSource = IOSurfaceAcceleratorGetRunLoopSource(accelerator); > + CFRunLoopAddSource(CFRunLoopGetMain(), runLoopSource, kCFRunLoopDefaultMode); Should we consider running this on another thread? Attachment 262458 [details] did not pass style-queue:
ERROR: Source/WebCore/platform/graphics/cocoa/IOSurface.mm:252: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
ERROR: Source/WebCore/platform/graphics/cocoa/IOSurface.mm:273: Place brace on its own line for function definitions. [whitespace/braces] [4]
ERROR: Source/WebCore/platform/graphics/cocoa/IOSurface.h:42: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/cocoa/IOSurface.h:44: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4]
ERROR: Source/WebCore/ChangeLog:10: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5]
ERROR: Source/WebCore/platform/spi/cocoa/IOSurfaceSPI.h:108: The parameter name "allocator" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/spi/cocoa/IOSurfaceSPI.h:109: The parameter name "accelerator" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/spi/cocoa/IOSurfaceSPI.h:119: The parameter name "accelerator" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 8 in 7 files
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #7) > Comment on attachment 262458 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=262458&action=review > > > Source/WebCore/ChangeLog:12 > > + Though the default is still RGBA, it is now possible to create and IOSurface > > "and" > Fixed! > > Source/WebCore/platform/graphics/cocoa/IOSurface.mm:259 > > + auto runLoopSource = IOSurfaceAcceleratorGetRunLoopSource(accelerator); > > + CFRunLoopAddSource(CFRunLoopGetMain(), runLoopSource, kCFRunLoopDefaultMode); > > Should we consider running this on another thread? I'll chat with Anders about this. Thanks! http://trac.webkit.org/changeset/190574 Comment on attachment 262458 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262458&action=review >>> Source/WebCore/platform/graphics/cocoa/IOSurface.mm:259 >>> + CFRunLoopAddSource(CFRunLoopGetMain(), runLoopSource, kCFRunLoopDefaultMode); >> >> Should we consider running this on another thread? > > I'll chat with Anders about this. > > Thanks! > > http://trac.webkit.org/changeset/190574 I think the important thing here is to know whether it uses the run loop that we install its source on for anything *other* than calling the callback. If not, then there's no need to move it to another thread, but if it does other work on that runloop we might want to. |