Lots of thumbnail requests with invalid size

I think this is actually on the Unity8 side rather than unity-scopes-shell.

The result cards are generated by CardCreator.js, which uses a CroppedImageMinimumSourceSize component to display the result art:

http://bazaar.launchpad.net/~unity-team/unity8/trunk/view/head:/plugins/Dash/CardCreator.js#L101

This component is implemented in QML but delegates finding the size of the image to the C++ component CroppedImageSizer

http://bazaar.launchpad.net/~unity-team/unity8/trunk/view/head:/plugins/Dash/CroppedImageMinimumSourceSize.qml

That class uses QNetworkAccessManager to try and get the file contents and uses a helper class to manage the response:

http://bazaar.launchpad.net/~unity-team/unity8/trunk/view/head:/plugins/Dash/croppedimagesizer.cpp#L102

That helper tries to read the returned data via QImageReader and passes the image size back to CroppedImageSizer:

http://bazaar.launchpad.net/~unity-team/unity8/trunk/view/head:/plugins/Dash/croppedimagesizerasyncworker.cpp#L58

If the worker provides a non-empty size, CroppedImageSizer then uses that size information to pick a sourceSize value that will result in the image being loaded at the correct size so it only has to be cropped in at most one dimension to fit. If an empty (or invalid) size is provided, it sets sourceSize to (0, 0):

http://bazaar.launchpad.net/~unity-team/unity8/trunk/view/head:/plugins/Dash/croppedimagesizer.cpp#L118

This causes a problem for the thumbnailer because QNetworkAccessManager has no idea how to deal with image:// URIs, so we get an invalid QSize from the worker and use a sourceSize of (0, 0). Perhaps it should just use the requested dimensions in this case instead. It doesn't look like there is a way to handle this crop mode properly with QQuickImageProvider.

I don't understand why you guys consider an undefined size to be invalid, the documentation of Qt is quite clear in that it is a valid size

****
sourceSize can be cleared to the natural size of the image by setting sourceSize to undefined.
****

So yes I want the natural size of the image if i can't find out what's the aspect ratio of the image. Besides it's weird because you complain for undefined but for 0x0 you give me a thumbnail just fine.

Well, consider the cost of scaling down from a full-size photo to a thumbnail. It takes a quarter of a second on a Nexus 4.

Surely, it's not too much to ask the caller what size thumbnail it wants? If we are not told what size the caller needs, we can either deliver the full-size image, which is really expensive and eats a lot of disk space, or we can deliver some thumbnail that is arbitrarily smaller and ends up getting scaled up and looking bad. Either the battery loses or the user experience loses.

The meaning of (0, 0) is "give me the best thumbnail available". We'll faithfully do that. We ask that callers use this feature judiciously because it is expensive. The meaning of (-1, -1) is "I don't know what I need, just give me something." I don't think that's a good idea, and we should never have supported that feature in the first place. If the caller doesn't know what size image it needs, there is a problem with the caller, I think.

If the caller doesn't mind scaling up and the loss of image quality, it is free to ask for (256, 256) or whatever, and we'll provide that. The point here is that the thumbnailer has no business setting policy. The policy is up to the caller.

> Well, consider the cost of scaling down from a full-size photo to a thumbnail. It takes a quarter of a second on a Nexus 4.
Yes, that should be done as few times as possible.

> Surely, it's not too much to ask the caller what size thumbnail it wants?
It actually is, i have no idea of the image i want unless you tell me the aspect ratio and since i can't know the aspect ratio i need the full image.

> The meaning of (-1, -1) is "I don't know what I need, just give me something.
You're making this up, the API documentation explicitly says "sourceSize can be cleared to the natural size of the image by setting sourceSize to undefined."

> The point here is that the thumbnailer has no business setting policy.
You say you have no business setting the policy but then go and set a policy that says "i'm not accepting undefined sizes even if the API says i should", please do not do that.

So, here is some history of this. The old thumbnailer, when presented with (-1,-1), delivered a 512 thumbnail. That wasn't really by design, but a side-effect of how the code was written. When we changed to the new thumbnailer, we decided to return the full-size image for an invalid QSize.

That turned out to be a mistake: when we ran a bunch of apps and scopes against the new thumbnailer, we found that there were lots of cases were the caller passed an uninitialized QSize, and we returned a full-size image in response. To the caller, everything appears to be working correctly, but it's working horribly inefficiently, both in terms of disk space and in terms of scaling.

If the caller wants a full-size image, it can do that, by asking for (0, 0). The advantage is that asking for a full-size image becomes an explicit operation, so it won't happen by accident.

I don't quite understand why you need to know the aspect ratio beforehand. Surely, there must be some display size into which the thumbnail is expected to fit? If we get something like QSize(512, 0) or (0, 512), we assume a 512x512 bounding box and scale accordingly. (We always preserve aspect ratio.) So, why not use that? And, if you really want a full-size image, just ask for (0,0) and you'll get it. But we can't return a full-size image for an uninitialized QSize; all that does is hide errors and it causes things to run horribly inefficiently.

Returning some other size for (-1,-1) will be wrong most of the time for most callers, no matter what size we pick. It'll be too small and look bad, or it'll be too large and inefficient.

Correction: the old thumbnailer returned 128x128 for QSize(-1,-1), not 512x512.

> I don't quite understand why you need to know the aspect ratio beforehand.
> Surely, there must be some display size into which the thumbnail is expected to fit?

Ok, let's give you an example.

I know i have to fit my image in something that is 100 width x 100 height and i am using the PreserveAspectCrop fill format meaning the image is scaled uniformly to fill, cropping if necessary

If i don't know the aspect ratio of the image that i have to draw which size do i request?

If the image is portrait i have to request (100, 0)

If the image is landscape i have to request (0, 100)

See http://paste.ubuntu.com/12197917/ using http://i.imgur.com/xiP0k4T.jpg as landscape.jpg and http://i.imgur.com/A2M6bnF.jpg as portrait.jpg and you'll see http://i.imgur.com/f5sEMhe.png. You can see there why we need to know the aspect ratio to achive the best quality. In the first row "Source Size Width" has the best quality while in the second row "Source Size Height" has the best quality.

Just to chime in, I believe this just shows an issue with the API as it stands today - it'd be best if we could supply the minimum width/height and have the other end (thumbnailer in this case) determine what to do.

But I agree with Albert, for now to comply with the API, undefined sourceSize needs to be supported, and return the natural image with no scaling.

https://code.launchpad.net/~aacid/unity8/cropped_image_unknown_aspect_ratio/+merge/269185 implements the suggest workaround by James, it's not terrible, but makes us use more memory than needed to get an acceptable quality.

To be clear, what I suggested was to use the height and width unchanged rather than multiplying by 4: that would improve the quality over the current code while not blowing out the memory requirements of providing huge images.

As a longer term solution to get images of the required quality, I suggested the following on IRC:

We've got enough information on the back end to provide an image of the proper size so it can be cropped to the desired dimensions without scaling. So the idea is to do something like this:

1. If the dash sees a URI like image://albumart, artistart or thumbnailer, add a crop=1 parameter to the end before using it as the source for the Image, and set sourceSize to the the cropped image dimensions.
2. In the image provider, if the crop parameter is included, interpret requestedSize as the desired crop dimensions rather than a bounding box.
3. return an image that can be cropped to those dimensions without scaling.

Qt already seems to handle the case when an image provider returns an image larger than requestedSize, so I imagine this would work okay.

This is a bit of a hack, but no more so than the whole CroppedImageMinimumSourceSize component. I agree that it'd be nice if Qt made it easier to load an image at an appropriate size to use with PreserveAspectCrop.

I haven't thought this through fully yet, but I think I agree with James. It should be possible for us to deliver an image that is x pixels wide/tall in its smaller dimension, rather than only delivering images that that are no bigger than x pixels in their larger dimension (which is what we are providing now).

I'd expect that we'd still return images that are possibly smaller than what was asked for (as we do now), because upscaling is a policy decision that should be up to the application, not the thumbnailer.

James and I can discuss this.

Ok, https://code.launchpad.net/~aacid/unity8/newCroppedImageMinimumSourceSize/+merge/269233 should solve the problems in the unity8 side

This bug was fixed in the package unity8 - 8.11+15.10.20150915-0ubuntu1

---------------
unity8 (8.11+15.10.20150915-0ubuntu1) wily; urgency=medium

  [ Albert Astals Cid ]
  * Fix testNotifications
  * Make the wait longer to make tests pass in CI
  * New simplified CroppedImageMinimumSourceSize (LP: #1467740)
  * Stop animateTimer when starting the fadeOutAnimation
  * Use AlreadyLaunchedUpstart so that tests passes
  * Workaround keyboard issues and make test pass again

  [ Daniel d'Andrada ]
  * DesktopStage: Refactor focus handling
  * Stabilize tstShell.test_launchedAppHasActiveFocus

  [ Gary.Wzl ]
  * Add widgetData["expanded"] property for expandable widget.

  [ Michael Terry ]
  * Fix typo that caused the tutorial to tease the launcher on top of
    the greeter.
  * Fix typo that caused the tutorial to tease the launcher on top of
    the greeter.

 -- Michael Zanetti <email address hidden> Tue, 15 Sep 2015 12:06:06 +0000