Merge lp://staging/~fboucault/thumbnailer/new_qml_api into lp://staging/thumbnailer

FAILED: Continuous integration, rev:137
http://jenkins.qa.ubuntu.com/job/thumbnailer-ci/136/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/thumbnailer-vivid-amd64-ci/30/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/thumbnailer-vivid-armhf-ci/28/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/thumbnailer-vivid-i386-ci/25/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/thumbnailer-ci/136/rebuild

PASSED: Continuous integration, rev:140
http://jenkins.qa.ubuntu.com/job/thumbnailer-ci/137/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-vivid-amd64-ci/31
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-vivid-armhf-ci/29
        deb: http://jenkins.qa.ubuntu.com/job/thumbnailer-vivid-armhf-ci/29/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-vivid-i386-ci/26

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/thumbnailer-ci/137/rebuild

PASSED: Continuous integration, rev:142
http://jenkins.qa.ubuntu.com/job/thumbnailer-ci/138/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-vivid-amd64-ci/32
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-vivid-armhf-ci/30
        deb: http://jenkins.qa.ubuntu.com/job/thumbnailer-vivid-armhf-ci/30/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-vivid-i386-ci/27

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/thumbnailer-ci/138/rebuild

I didn't get to review the functional bits quite yet, but here are some simpler issues in the mean time.

- the qml tests don't seem to be run
- the bit that converts relative file names to absolute ones is duplicated in get_thumbnail and needs_generation, please move it to a shared function in an anonymous namespace
- the name needs_generation seems a bit backwards, would it be better to call it is_cached instead

Qml tests not being run automatically is not a new issue. Should be fixed
separately.
Le 25 févr. 2015 07:52, "Jussi Pakkanen" <email address hidden> a
écrit :

> Review: Needs Fixing
>
> I didn't get to review the functional bits quite yet, but here are some
> simpler issues in the mean time.
>
> - the qml tests don't seem to be run
> - the bit that converts relative file names to absolute ones is duplicated
> in get_thumbnail and needs_generation, please move it to a shared function
> in an anonymous namespace
> - the name needs_generation seems a bit backwards, would it be better to
> call it is_cached instead
>
> --
> https://code.launchpad.net/~fboucault/thumbnailer/new_qml_api/+merge/250832
> You are the owner of lp:~fboucault/thumbnailer/new_qml_api.
>

Other minor changes:

 - The new added symbol:
+ (c++)"Thumbnailer::thumbnail_needs_generation(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, ThumbnailSize)@Base" 1.1

The version should be 0replaceme instead of 1.1, so the debian machinery sets the right value.

 - In the newly created file qthumbnailer.h members should follow the same rules. Either to have the m_/s_ prefix or none. The first class have prefixes while the second doesn't.

> Other minor changes:
>
> - The new added symbol:
> + (c++)"Thumbnailer::thumbnail_needs_generation(std::basic_string<char,
> std::char_traits<char>, std::allocator<char> > const&, ThumbnailSize)@Base"
> 1.1
>
> The version should be 0replaceme instead of 1.1, so the debian machinery sets
> the right value.
>

Fixed.

> - In the newly created file qthumbnailer.h members should follow the same
> rules. Either to have the m_/s_ prefix or none. The first class have prefixes
> while the second doesn't.

Fixed.

> I didn't get to review the functional bits quite yet, but here are some
> simpler issues in the mean time.
>
> - the qml tests don't seem to be run
> - the bit that converts relative file names to absolute ones is duplicated in
> get_thumbnail and needs_generation, please move it to a shared function in an
> anonymous namespace

Fixed.

> - the name needs_generation seems a bit backwards, would it be better to call
> it is_cached instead

Fixed.

PASSED: Continuous integration, rev:147
http://jenkins.qa.ubuntu.com/job/thumbnailer-ci/139/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-vivid-amd64-ci/33
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-vivid-armhf-ci/31
        deb: http://jenkins.qa.ubuntu.com/job/thumbnailer-vivid-armhf-ci/31/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-vivid-i386-ci/28

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/thumbnailer-ci/139/rebuild

To me this seems a fix on the wrong level, that is, it'll mean people have to use Thumbnail {} instead of Image {}, ideally we could still use Image {} and have parallization at the Qt level, though obviously that means convincing the Qt people that this makes sense and getting it accepted upstream.

Right, +1 to the above. Can you mark this as being a workaround for QTBUG-37988 please.

> To me this seems a fix on the wrong level, that is, it'll mean people have to
> use Thumbnail {} instead of Image {}, ideally we could still use Image {} and
> have parallization at the Qt level, though obviously that means convincing the
> Qt people that this makes sense and getting it accepted upstream.

No matter how much we patch Qt it will never do the right thing, ie. when a thumbnail is being generated, loading thumbnails that have already been generated previously should not be blocked.

Let me try to condense Florian's concerns for those who have not had to deal with the process of thumbnailing.

Creating a video screenshot thumbnail takes several seconds (up to 5) on the device.

Creating a thumbnail for other types (mostly images) takes a fraction of a second. Loading a preexisting thumbnail takes roughly the same.

Suppose you have n threads for thumbnailing and want to display n videos and m images. If you submit the videos first, the images are stuck until the first video gets processed.

What we want is to be able to submit both images and videos and have the system prioritise those thumbnails that are quick to load (like, say, reserving one thread for them and one for video screenshots).

Qt's image provider does not have priorisation capabilities and to fix our use case something like that needs to be added to Qt. This is the Correct thing to do, but on the other hand it is a lot more work than just increasing the amount of threads for QQuickImageProviders.

> Let me try to condense Florian's concerns for those who have not had to deal
> with the process of thumbnailing.
>
> Creating a video screenshot thumbnail takes several seconds (up to 5) on the
> device.
>
> Creating a thumbnail for other types (mostly images) takes a fraction of a
> second. Loading a preexisting thumbnail takes roughly the same.
>
> Suppose you have n threads for thumbnailing and want to display n videos and m
> images. If you submit the videos first, the images are stuck until the first
> video gets processed.
>
> What we want is to be able to submit both images and videos and have the
> system prioritise those thumbnails that are quick to load (like, say,
> reserving one thread for them and one for video screenshots).
>
> Qt's image provider does not have priorisation capabilities and to fix our use
> case something like that needs to be added to Qt. This is the Correct thing to
> do, but on the other hand it is a lot more work than just increasing the
> amount of threads for QQuickImageProviders.

Yes and no. I am also fixing a case that you merged with another one as being the same:
case 1) image with non existing thumbnail
case 2) video with non existing thumbnail
case 3) any media with already existing thumbnail

1) is fast but not instant
2) is slow
3) is instant

But because of the queuing, if a video is being thumbnailed, 1) and 3) become slow.
Adding N more threads to the image provider infrastructure would fix the issue of 1) becoming slow but 3) would still not be instant in some cases.

LGTM. One thing to note is that there are two queues called video and image. The image queue deals with other image types besides images, such as audio files (and possibly in the future also pdf files, libreoffice docs etc). Thus it might make sense to rename it s_otherQueue, s_basicQueue or something like that. This is totally up to you, though, and not a requirement.

Will be reworked in 1-2 weeks to make use of the new Qt QQuickImageProvider async API that aacid is working on. That will allow us to not have that new Thmubnailer {} QML API.

Rejecting this, seeing it is obsolete.