Thumbnailer

Merge lp://staging/~michihenning/thumbnailer/vs-thumb-refactor into lp://staging/thumbnailer/devel

vs-thumb-refactor
Merge into devel

Proposed by Michi Henning on 2015-10-14

Status:	Merged
Approved by:	James Henstridge on 2015-10-23
Approved revision:	309
Merged at revision:	289
Proposed branch:	lp://staging/~michihenning/thumbnailer/vs-thumb-refactor
Merge into:	lp://staging/thumbnailer/devel
Prerequisite:	lp://staging/~michihenning/thumbnailer/qt-rate-limiter
Diff against target:	1439 lines (+512/-335) 14 files modified CMakeLists.txt (+3/-1) debian/control (+1/-0) include/internal/imageextractor.h (+2/-3) src/imageextractor.cpp (+25/-14) src/thumbnailer.cpp (+1/-1) src/trace.cpp (+1/-1) src/vs-thumb/CMakeLists.txt (+25/-13) src/vs-thumb/thumbnailextractor.cpp (+244/-205) src/vs-thumb/thumbnailextractor.h (+32/-3) src/vs-thumb/vs-thumb.cpp (+18/-42) tests/testsetup.h.in (+2/-0) tests/thumbnailer/thumbnailer_test.cpp (+14/-3) tests/vs-thumb/CMakeLists.txt (+7/-3) tests/vs-thumb/vs-thumb_test.cpp (+137/-46)
To merge this branch:	bzr merge lp://staging/~michihenning/thumbnailer/vs-thumb-refactor
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
James Henstridge		2015-10-14	Approve on 2015-10-23
PS Jenkins bot (community)	continuous-integration		Approve on 2015-10-22
Review via email: mp+274346@code.staging.launchpad.net

Commit message

Partially refactored vs-thumb. Better performance, tracing, and error messages.

Description of the change

Stage 1 of vs-thumb refactoring.

Changed thumbnail extraction to return tiff image via a pipe. This speeds up MP3 extraction by 60%.

Bundled vs-thumb code into a static library because compiling the extractor source separately for vs-thumb and the tests caused gcovr to get confused.

Changed cover art extraction to a non-member function for clarity.

Un-pimpled thumbnail extractor class.

Better tracing with timestamps and more informative error messages.

More tests and much improved test coverage.

Many minor style fixes.

Still to do for stage 2:

- Re-factor code some more. (Some functions are rather large; could possibly use virtual base methods instead of if-then-else in some places.)

- We are still doing more image conversions than strictly necessary. It would be better to write a header into the pipe that describes the encoding for the image, so we don't needlessly encode from jpg to tiff, and then back to jpg in calling process, for example).

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-10-14:

FAILED: Continuous integration, rev:307
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~michihenning/thumbnailer/vs-thumb-refactor/+merge/274346/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-ci/490/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-vivid-amd64-ci/195/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-vivid-armhf-ci/194/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-vivid-i386-ci/196/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-amd64-ci/300/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-armhf-ci/305/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-i386-ci/299/console

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

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-10-14:

FAILED: Continuous integration, rev:308
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~michihenning/thumbnailer/vs-thumb-refactor/+merge/274346/+edit-commit-message

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

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-10-16:

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

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-10-16:

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

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-10-16:

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

review: Approve (continuous-integration)

Revision history for this message

James Henstridge (jamesh) wrote on 2015-10-21:

This looks pretty good. At first I thought there might be some issue with handling stdout/stderr, but the docs seem to indicate that QProcess will buffer all the output so that shouldn't be a problem.

It looks like any output to stderr from vs-thumb will be discarded on a successful thumbnail. This is probably okay, but might be a bit annoying if following the logs while trying to determine why vs-thumb is stuck somewhere.

I've left some extra inline comments, but they don't really find fault with any of the logic.

review: Needs Information

lp://staging/~michihenning/thumbnailer/vs-thumb-refactor updated on 2015-10-22

309. By Michi Henning on 2015-10-22: Addressed review comments from James.

Revision history for this message

Michi Henning (michihenning) wrote on 2015-10-22:

> It looks like any output to stderr from vs-thumb will be discarded on a
> successful thumbnail. This is probably okay, but might be a bit annoying if
> following the logs while trying to determine why vs-thumb is stuck somewhere.

Good point, thanks! I've changed it to leave stderr alone, so anything written by vs-thumb ends up going to the log file immediately.

I renamed data() to read() and added an assertion that triggers if we call it a second time.

For the .tiff extension, I changed this after I noticed that we still had a bunch of tests that incorrectly used a .jpg extension. In turn, that caused problems when I looked at vs-thumb output with a browser (or image viewer of some kind, I can't remember the details). Also, when testing, I quite often run vs-thumb manually. So, I added this mainly as a convenience. I don't think it does any harm?

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-10-22:

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

review: Approve (continuous-integration)

Revision history for this message

James Henstridge (jamesh) wrote on 2015-10-23:

I guess the filename change doesn't really matter that much, since we don't use that mode in production.

It still has a bit of a bad smell about it. I'd be happier if we always appended ".tiff" (in which case we never write to the requested file), or never append the extension (in which case we always write to the requested file).

review: Approve

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:

Michi Henning

Unity Team