Merge lp://staging/~michihenning/thumbnailer/no-tmp-file into lp://staging/thumbnailer/devel

Proposed by Michi Henning
Status: Merged
Approved by: Michi Henning
Approved revision: 309
Merged at revision: 306
Proposed branch: lp://staging/~michihenning/thumbnailer/no-tmp-file
Merge into: lp://staging/thumbnailer/devel
Diff against target: 1214 lines (+172/-284)
24 files modified
include/internal/file_io.h (+8/-1)
include/internal/image.h (+2/-1)
include/internal/imageextractor.h (+1/-1)
include/internal/thumbnailer.h (+1/-1)
plugins/Ubuntu/Thumbnailer.0.1/thumbnailerimageresponse.cpp (+0/-1)
src/artgeneratorcommon.cpp (+0/-72)
src/file_io.cpp (+14/-4)
src/image.cpp (+6/-0)
src/imageextractor.cpp (+2/-13)
src/libthumbnailer-qt/CMakeLists.txt (+1/-2)
src/libthumbnailer-qt/libthumbnailer-qt.cpp (+7/-9)
src/service/dbusinterface.cpp (+10/-11)
src/service/dbusinterface.h (+3/-4)
src/service/dbusinterface.xml (+3/-6)
src/service/handler.cpp (+34/-105)
src/service/handler.h (+3/-4)
src/thumbnailer-admin/get_local_thumbnail.cpp (+2/-2)
src/thumbnailer-admin/get_remote_thumbnail.cpp (+2/-2)
src/thumbnailer.cpp (+4/-4)
tests/dbus/dbus_test.cpp (+20/-20)
tests/file_io/file_io_test.cpp (+11/-5)
tests/stress/stress_test.cpp (+24/-0)
tests/thumbnailer-admin/thumbnailer-admin_test.cpp (+3/-5)
tests/thumbnailer/thumbnailer_test.cpp (+11/-11)
To merge this branch: bzr merge lp://staging/~michihenning/thumbnailer/no-tmp-file
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Approve
Michi Henning (community) Approve
James Henstridge Approve
Review via email: mp+278924@code.staging.launchpad.net

Commit message

Replaced returning of thumbnails via file descriptor with returning the image contents
directly over DBus. About 7-8% performance improvement for hits on 128-size thumbnails.

Description of the change

Replaced returning of thumbnails via file descriptor with returning the image contents
directly over DBus. About 7-8% performance improvement for hits on 128-size thumbnails.

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
James Henstridge (jamesh) wrote :

This looks pretty good. Getting rid of the thumbnailer client side image scaling code seems like a reasonable change.

I've left a few inline comments on the diff.

review: Needs Fixing
Revision history for this message
Michi Henning (michihenning) wrote :

Thanks for the thorough review! I've addressed all your points, except for the annotation. I have no opinion either way so, if you think it's better to remove the annotation, that's cool. I just thought that having it might be useful for some tool or other. (The doc is not clear on when you have to have the annotation and when it's optional.)

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
James Henstridge (jamesh) wrote :

I think it'd be better to remove the annotation in that case. "ay" literally means "array of bytes", so we're not really adding any semantic information.

The annotation for the QSize arguments is there because this isn't the default marshalling: a struct of two integers could be anything, so in this case the annotation is actually required and provides new information.

Revision history for this message
Michi Henning (michihenning) wrote :

OK, cool, I've removed the annotations.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
James Henstridge (jamesh) wrote :

Looks good.

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Michi Henning (michihenning) wrote :

Looks the vivid amd-64 builder was hopelessly overloaded. Re-approving for Jenkins.

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Michi Henning (michihenning) wrote :

Re-approving for Jenkins.

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) :
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: