Bug 162109 – The dragged image should be the current frame only of the animated image

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Said Abou-Hallawa]

The animated image can have many frames. Decoding all of the frames, getting them as tiffRepresentation then and converting them to an NSImage is needlessly expensive. The dragged image does not animate anyway when dragging it so it make sense to get the first frame only.

In BitmapImage.h there is a comment saying that the member m_nsImage is a cached NSImage of frame 0. This comment is no longer correct. But it seems this was the intention for this member and later it was changed.

The fix to have two functions in Image and BitmapImage which both return NSImage. The first returns the all the frames and the second returns the first frame only.

[Said Abou-Hallawa]

Created attachment 289154 [details]
Patch

[Sam Weinig]

Is the first frame the best? Maybe the current frame would make more sense? Also, is there path forward where we can actually show the image animating in the drag?

[Said Abou-Hallawa]

(In reply to comment #2)
> Is the first frame the best? Maybe the current frame would make more sense?
> Also, is there path forward where we can actually show the image animating
> in the drag?

Yes returning the current frame might be better than the first frame. But this means we can't cache its NSImage. Caching the first frame or the last dragged frame are almost the same if the user drags the same image but at a different frame.

Animating the drag image needs a timer and some mechanism to keep updating the drag image which does not exist right now. But if we do it, this will lead us to the same problem we are trying to fix here. We can't decode all the frames and hand them as an animated drag image. This would require a lot of memory and CPU time to do all in one time. The drag image has to accept an encoded image format and be responsible of decoding and freeing its frames when needed. This seems a lot of work and I am not sure if the scenario will be beneficial at the end or not.

[Said Abou-Hallawa]

Created attachment 290029 [details]
Patch

[Build Bot]

Comment on attachment 290029 [details]
Patch

Attachment 290029 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2158068

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 290037 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 290029 [details]
Patch

Attachment 290029 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2158145

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 290042 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 290029 [details]
Patch

Attachment 290029 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2158214

New failing tests:
http/tests/security/dataTransfer-set-data-file-url.html
http/tests/misc/drag-over-iframe-invalid-source-crash.html
imported/blink/compositing/drag-opacity-crash.html
editing/pasteboard/can-read-in-dragstart-event.html
media/video-controls-drag.html
fast/forms/textfield-drag-into-disabled.html
editing/pasteboard/dataTransfer-setData-getData.html
editing/pasteboard/drop-text-events-sideeffect-crash.html
editing/selection/drag-in-iframe.html
editing/pasteboard/drop-text-events-sideeffect.html
editing/selection/crash-on-drag-with-mutation-events.html
editing/pasteboard/drag-drop-to-data-url.html
fast/events/setDragImage-in-document-element-crash.html
imported/blink/fast/events/drag-leak-document.html
fast/events/dragging-mouse-moves.html
fast/css/user-drag-none.html
editing/pasteboard/drag-drop-iframe-refresh-crash.html
fast/css-generated-content/drag-state.html
http/tests/misc/dragging-large-animated-image.html
http/tests/security/drag-drop-different-origin.html
fast/events/drag-and-drop-autoscroll-inner-frame.html
editing/pasteboard/drag-drop-input-in-svg.svg
editing/selection/drag-start-event-client-x-y.html

[Build Bot]

Created attachment 290046 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 290029 [details]
Patch

Attachment 290029 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2158353

New failing tests:
http/tests/misc/dragging-large-animated-image.html

[Build Bot]

Created attachment 290048 [details]
Archive of layout-test-results from ews125 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Said Abou-Hallawa]

Created attachment 290106 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 290106 [details]
Patch

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

> Source/WebKit/mac/ChangeLog:1
> [phone]  Said Abou-Hallawa  <[e-mail]>

Double changelog entry.

> LayoutTests/http/tests/misc/dragging-large-animated-image.html:28
> +            debug(time < 50 ? "Succeeded." : "Failed.");

I think this test is likely to be flakey on bots. Those machines can be very loaded and individual test runners are often CPU-starved.

[Tim Horton]

Comment on attachment 290106 [details]
Patch

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

> Source/WebCore/platform/graphics/BitmapImage.h:93
> +    NSImage* nsImage() override;

Star's on the wrong side.

> Source/WebCore/platform/graphics/BitmapImage.h:94
> +    RetainPtr<NSImage> currentFrameNSImage() override;

I wonder if we should swap these ("nsImageIncludingAllFrames" and "nsImage") to make the sane thing the default, and the expensive thing very clear.

[Said Abou-Hallawa]

Created attachment 290265 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 290106 [details]
Patch

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

>> Source/WebKit/mac/ChangeLog:1
>> [phone]  Said Abou-Hallawa  <[e-mail]>
> 
> Double changelog entry.

Fixed.

>> Source/WebCore/platform/graphics/BitmapImage.h:93
>> +    NSImage* nsImage() override;
> 
> Star's on the wrong side.

I think the star is on the right side.

>> Source/WebCore/platform/graphics/BitmapImage.h:94
>> +    RetainPtr<NSImage> currentFrameNSImage() override;
> 
> I wonder if we should swap these ("nsImageIncludingAllFrames" and "nsImage") to make the sane thing the default, and the expensive thing very clear.

I am not sure about that. I would expect BitmapImage::nsImage() to return whatever BitmapImage holds but as an NSImage. If has one or more frames, all the frames should be included. But if I need something special like an NSImage with the current frame only, then I would expect the name to be more specific.

>> LayoutTests/http/tests/misc/dragging-large-animated-image.html:28
>> +            debug(time < 50 ? "Succeeded." : "Failed.");
> 
> I think this test is likely to be flakey on bots. Those machines can be very loaded and individual test runners are often CPU-starved.

I could not come up with a reliable way to make this test works, so I deleted it.

[Tim Horton]

> 
> >> Source/WebCore/platform/graphics/BitmapImage.h:93
> >> +    NSImage* nsImage() override;
> > 
> > Star's on the wrong side.
> 
> I think the star is on the right side.

Nope, it's not. C++ on the left, ObjC on the right.

> >> Source/WebCore/platform/graphics/BitmapImage.h:94
> >> +    RetainPtr<NSImage> currentFrameNSImage() override;
> > 
> > I wonder if we should swap these ("nsImageIncludingAllFrames" and "nsImage") to make the sane thing the default, and the expensive thing very clear.
> 
> I am not sure about that. I would expect BitmapImage::nsImage() to return
> whatever BitmapImage holds but as an NSImage. If has one or more frames, all
> the frames should be included. But if I need something special like an
> NSImage with the current frame only, then I would expect the name to be more
> specific.

In that case, I think we should look through all other callers and see who else wants the image including all frames.

[Tim Horton]

(In reply to comment #18)
> > 
> > >> Source/WebCore/platform/graphics/BitmapImage.h:93
> > >> +    NSImage* nsImage() override;
> > > 
> > > Star's on the wrong side.
> > 
> > I think the star is on the right side.
> 
> Nope, it's not. C++ on the left, ObjC on the right.
> 
> > >> Source/WebCore/platform/graphics/BitmapImage.h:94
> > >> +    RetainPtr<NSImage> currentFrameNSImage() override;
> > > 
> > > I wonder if we should swap these ("nsImageIncludingAllFrames" and "nsImage") to make the sane thing the default, and the expensive thing very clear.
> > 
> > I am not sure about that. I would expect BitmapImage::nsImage() to return
> > whatever BitmapImage holds but as an NSImage. If has one or more frames, all
> > the frames should be included. But if I need something special like an
> > NSImage with the current frame only, then I would expect the name to be more
> > specific.
> 
> In that case, I think we should look through all other callers and see who
> else wants the image including all frames.

A cursory glance suggests that more of them should probably switch, and it's not totally clear that any of them (except maybe the WebIconDatabase caller) really want the all-frames-in-one-NSImage madness.

[Build Bot]

Comment on attachment 290265 [details]
Patch

Attachment 290265 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2171559

New failing tests:
fast/images/pixel-crack-image-background-webkit-transform-scale.html

[Build Bot]

Created attachment 290274 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Said Abou-Hallawa]

Created attachment 290362 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 290362 [details]
Patch

Clearing flags on attachment: 290362

Committed r206683: <http://trac.webkit.org/changeset/206683>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Said Abou-Hallawa]

<rdar://problem/28319987>

[Simon Fraser (smfr)]

I think this broke an API test:
FAIL WebKit2.FindMatches

/Volumes/Data/slave/elcapitan-debug/build/Tools/TestWebKitAPI/Tests/WebKit2/FindMatches.mm:108
Value of: [[expectedImage.representations objectAtIndex:0] isKindOfClass:[NSBitmapImageRep class]]
  Actual: false
Expected: true

[Tim Horton]

(In reply to comment #26)
> I think this broke an API test:
> FAIL WebKit2.FindMatches
> 
> /Volumes/Data/slave/elcapitan-debug/build/Tools/TestWebKitAPI/Tests/WebKit2/
> FindMatches.mm:108
> Value of: [[expectedImage.representations objectAtIndex:0]
> isKindOfClass:[NSBitmapImageRep class]]
>   Actual: false
> Expected: true

There's a pretty good chance that means it broke Mail find in page, too, then.

[Chris Dumez]

I think this patch may have caused an API test failure:
FAIL WebKit2.FindMatches

/Volumes/Data/slave/elcapitan-release/build/Tools/TestWebKitAPI/Tests/WebKit2/FindMatches.mm:108
Value of: [[expectedImage.representations objectAtIndex:0] isKindOfClass:[NSBitmapImageRep class]]
  Actual: false
Expected: true

[Ryan Haddad]

(In reply to comment #27)
> (In reply to comment #26)
> > I think this broke an API test:
> > FAIL WebKit2.FindMatches
> > 
> > /Volumes/Data/slave/elcapitan-debug/build/Tools/TestWebKitAPI/Tests/WebKit2/
> > FindMatches.mm:108
> > Value of: [[expectedImage.representations objectAtIndex:0]
> > isKindOfClass:[NSBitmapImageRep class]]
> >   Actual: false
> > Expected: true
> 
> There's a pretty good chance that means it broke Mail find in page, too,
> then.

In that case I'll go ahead and roll this out.

[Ryan Haddad]

Reverted r206683 for reason:

This change caused API test WebKit2.FindMatches to fail on Mac.

Committed r206720: <http://trac.webkit.org/changeset/206720>

[Said Abou-Hallawa]

Created attachment 290499 [details]
Patch

[Said Abou-Hallawa]

(In reply to comment #26)
> I think this broke an API test:
> FAIL WebKit2.FindMatches
> 
> /Volumes/Data/slave/elcapitan-debug/build/Tools/TestWebKitAPI/Tests/WebKit2/
> FindMatches.mm:108
> Value of: [[expectedImage.representations objectAtIndex:0]
> isKindOfClass:[NSBitmapImageRep class]]
>   Actual: false
> Expected: true

Using [NSImage initWithCGImage] creates an image with NSImageRep of class NSCGImageSnapshotRep. The expected type is NSBitmapImageRep. To get this representation, the easiest way is to get a tiffRepresentation for the current frame and then use [NSImage initWithData] to create the NSImage.

[Tim Horton]

(In reply to comment #32)
> (In reply to comment #26)
> > I think this broke an API test:
> > FAIL WebKit2.FindMatches
> > 
> > /Volumes/Data/slave/elcapitan-debug/build/Tools/TestWebKitAPI/Tests/WebKit2/
> > FindMatches.mm:108
> > Value of: [[expectedImage.representations objectAtIndex:0]
> > isKindOfClass:[NSBitmapImageRep class]]
> >   Actual: false
> > Expected: true
> 
> Using [NSImage initWithCGImage] creates an image with NSImageRep of class
> NSCGImageSnapshotRep. The expected type is NSBitmapImageRep. To get this
> representation, the easiest way is to get a tiffRepresentation for the
> current frame and then use [NSImage initWithData] to create the NSImage.

Oh! Very possible that this is OK as-is, in that case. You should test find-in-Mail and consider just adjusting the test.

[Said Abou-Hallawa]

(In reply to comment #33)
> (In reply to comment #32)
> > (In reply to comment #26)
> > > I think this broke an API test:
> > > FAIL WebKit2.FindMatches
> > > 
> > > /Volumes/Data/slave/elcapitan-debug/build/Tools/TestWebKitAPI/Tests/WebKit2/
> > > FindMatches.mm:108
> > > Value of: [[expectedImage.representations objectAtIndex:0]
> > > isKindOfClass:[NSBitmapImageRep class]]
> > >   Actual: false
> > > Expected: true
> > 
> > Using [NSImage initWithCGImage] creates an image with NSImageRep of class
> > NSCGImageSnapshotRep. The expected type is NSBitmapImageRep. To get this
> > representation, the easiest way is to get a tiffRepresentation for the
> > current frame and then use [NSImage initWithData] to create the NSImage.
> 
> Oh! Very possible that this is OK as-is, in that case. You should test
> find-in-Mail and consider just adjusting the test.

It looks like we have to create the NSImage dragged image from the data of the frames (like what I did in the last patch).

I tried the CGImageRef solution and I fixed the NSImageRep issue in the test. When trying the test on the Retina display, it fails again but with the image size:

/Volumes/Data/WebKit/OpenSource/Tools/TestWebKitAPI/Tests/WebKit2/FindMatches.mm:113
Value of: expectedWidth
  Actual: 144
Expected: size.width
Which is: 72


/Volumes/Data/WebKit/OpenSource/Tools/TestWebKitAPI/Tests/WebKit2/FindMatches.mm:114
Value of: expectedHeight
  Actual: 72
Expected: size.height
Which is: 36

[Said Abou-Hallawa]

I tested dragging test selection, non-animated images and animated images in WK1 and WK2 on both 1x and 2x and everything seems fine. Also I ran the FindMatches test on 1x and w2 and it passed on both. I made sure find in Mail works as expected.

[Tim Horton]

Comment on attachment 290499 [details]
Patch

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

> Source/WebCore/platform/graphics/mac/ImageMac.mm:137
> +    return adoptNS([[NSImage alloc] initWithData:(NSData*)data.get()]);

Space before the star.

[WebKit Commit Bot]

Comment on attachment 290499 [details]
Patch

Clearing flags on attachment: 290499

Committed r206802: <http://trac.webkit.org/changeset/206802>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.