[dolphin] Animate gifs on preview
ClosedPublic

Authored by iasensio on Aug 28 2019, 3:58 PM.

Details

Summary

Adds the capability to view animated images on the preview in the information panel.
This was a request from a user back in 2009 (https://bugs.kde.org/show_bug.cgi?id=182257), but I think nowadays with stickers/memes and what not, it's even more useful.
It keeps the size default transition of the preview viewer before starting the animation, so that the visual integration is smoother.

BUG: 182257

Test Plan

Open the information panel and hover over some animated images (gif/webp/mng)

Diff Detail

Repository
R318 Dolphin
Branch
animate_gif_preview
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 15951
Build 15969: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes

I also added stopAnimatedImage() because stopping it by passing an empty filename was kind of hacky

src/panels/information/pixmapviewer.cpp
46

Should it also be destroyed on animation stopping? Or only if a different non-animated item gets loaded? Or that would be an extra penalty in creation/destruction.

ngraham added inline comments.Aug 29 2019, 3:26 PM
src/panels/information/pixmapviewer.cpp
119

return QString() instead of QString::null

137

Need to move the check for m_animatedImage separately since if it doesn't already exist, the call to m_animatedImage->state() will crash

166

ditto

iasensio updated this revision to Diff 64955.Aug 29 2019, 4:29 PM
iasensio marked an inline comment as done.
  • Return QSring() instead of QString::null

About checking for m_animatedImage

src/panels/information/pixmapviewer.cpp
137

It's a common pattern in plain C. If m_animatedImage is evaluated false, it doesn't check the rest of the condition. It doesn't crash now for me but I'm guessing that it's not a good pattern in C++. Any ideas on how to do it while keeping it simple?
Thanks for the patience 😃

ngraham added inline comments.Aug 29 2019, 4:30 PM
src/panels/information/pixmapviewer.cpp
137

You're probably right and I'm being dense

ngraham added a comment.EditedAug 29 2019, 4:35 PM

Nice. Testing it out, I found some bugs:

  • Hover over an animated gif, then hover over a non-animated gif, then hover back over the original animated gif -> no animation displayed in the preview
  • Hover over an animated gif and then click on it -> animation stops
  • Hover over an animated gif and then hover over a jpeg -> preview briefly updates to show the jpeg, then shows the animated gif again
  • Hover over an animated gif then then click on a jpeg image, then click on another jpeg image -> while the mouse is between the image, the animated gif plays in the information panel
ngraham requested changes to this revision.Aug 29 2019, 4:38 PM
This revision now requires changes to proceed.Aug 29 2019, 4:38 PM

[spam comment removed by sysadmin]

[spam comment removed by sysadmin]

[spam comment removed by sysadmin]

iasensio marked 3 inline comments as done.EditedAug 29 2019, 5:49 PM

I've reviewed your test cases and now all of them are (hopefully) gone. Also another one when selecting multiple files.
Finally I had to go deleting the QMovieobject, because it doesn't provide a way to deactivate it, other than setting an empty FileName (which shows a log error message).

iasensio updated this revision to Diff 64959.Aug 29 2019, 5:50 PM
  • Fix bugs when hovering and selecting
meven added inline comments.Aug 29 2019, 6:02 PM
src/panels/information/pixmapviewer.cpp
46

I don't think it is necessary.
Once this has been initiated we have good chances that it will serve again.
And keeping it is just a little memory occupied while destroying will use some CPU and some more if and when the animatedImage is used again.

You can replace m_animatedImage = nullptr; here with a declaration in the constructor :

PixmapViewer::PixmapViewer(QWidget* parent, Transition transition) :
    QWidget(parent),
    m_transition(transition),
    m_animationStep(0),
    m_sizeHint(),
    m_animatedImage(nullptr)

I am not even sure it is necessary to declare it at all.

Excellent. There's just one remaining issue I can see right now: clicking to select a hovered gif still restarts the animation. It should not.

meven added inline comments.Aug 29 2019, 7:54 PM
src/panels/information/pixmapviewer.cpp
111

After @ngraham last comment
Just check the fileName has changed before stopping and re-setting the field

iasensio updated this revision to Diff 64967.Aug 29 2019, 8:07 PM
iasensio marked 2 inline comments as done.
  • Check if same file or already running
iasensio updated this revision to Diff 64970.Aug 29 2019, 8:57 PM
  • Avoid restarting animation on hover+select
pino added a subscriber: pino.Aug 29 2019, 9:05 PM
pino added inline comments.
src/panels/information/informationpanelcontent.cpp
102

The mimetype for MNG images is video/x-mng. not image/mng.
In general, there's nothing that ensures that for the "foo" QMovie type the corresponding mime is image/foo -- it could be very well image/x-foo for example (in case of a non-IANA mimetype).

iasensio updated this revision to Diff 64972.Aug 29 2019, 10:18 PM
iasensio marked an inline comment as done.
  • Fix animated mimetypes (harcoded)

Thanks @pino!

I've finally moved it to a separate function, and directly harcoded the mimetypes, as it seems cleaner.
I've not been able to test if the preview of mngfiles actually animates or not, but the mimetype check is now correct.

I couldn't find a mng file all over the internet, and the one I created using convert do not animate in any of the viewers I could find. I looks like an extinct format, and there is also this: QMovie no longer supports .mng. However, my QMovie::supportedFormats() does actually return mng.

If anyone can test it with a trusted file and environment it would be great.

ngraham added inline comments.Aug 30 2019, 5:38 PM
src/panels/information/informationpanelcontent.cpp
173

This change will result in the preview being refreshed a lot more, even if it's not an animated image. Can you find a way to do what you're trying to do without changing this?

Yes, I was reluctant to move this line inside the condition, but it will actually refresh the preview less, that is, no refreshing when the item to be shown is the same as the current one. In fact, this change is not needed to avoid the animation restarting, but to avoid the following flickering:

.

With the refreshPreview() call outside the condition, when showItem() is called twice in a row on the same item (by hovering and selecting, for example), a new thumbnail job starts, and a new static pixmap is sent to the preview. With static images, there is no issue (since it is the same image rendered).

I've tested the cases with and without animation, and even media, and I found no problems, but it is possible that there is a use case for that extra refreshing outside the condition. Would it be possible to track the code changes to know why is there? Otherwise, to avoid messing a lot with the code to detect and stop that extra refresh only for the animated image case, I would go either with the current change or the restart behavior.

ngraham accepted this revision as: VDG, ngraham.Aug 30 2019, 7:14 PM

Yeah, I see now that you're right.

This looks great to me now. @elvisangelaccio?

iasensio updated this revision to Diff 65048.Aug 31 2019, 12:30 PM
iasensio marked an inline comment as done.
  • Fix refresh on resizing event
iasensio added a comment.EditedAug 31 2019, 12:49 PM

Yeah, I see now that you're right.

This looks great to me now. @elvisangelaccio?

Well, you were right for pointing it out, since it would have been a regression. Thanks!
I've found that the reason to refresh the pixmap even on the same item is to handle resizing the panel (D22473).

Now I finally managed to avoid the restarting and flickering on hover+select, and not breaking the resize update. The animated image will restart on panel resizing thought, but this is for the best, since we have the nice transition animation.

meven added a comment.Aug 31 2019, 5:41 PM

Yeah, I see now that you're right.

This looks great to me now. @elvisangelaccio?

Well, you were right for pointing it out, since it would have been a regression. Thanks!
I've found that the reason to refresh the pixmap even on the same item is to handle resizing the panel (D22473).

Well you pointed right at a weakness in the code : resizing the widget and refreshing its content use the same code path.
This is something I am thinking about refactoring since just like here, it is causing bugs and is not easily maintainable.
But only after D22183 has been merged.

elvisangelaccio requested changes to this revision.Sep 1 2019, 12:40 PM

Thanks for the patch. Please rebase it on top of D22183 (or wait until we merge it, should happen soon).

This revision now requires changes to proceed.Sep 1 2019, 12:40 PM
iasensio updated this revision to Diff 65139.Sep 1 2019, 4:00 PM
  • Rebased to master after D22183
iasensio updated this revision to Diff 65141.Sep 1 2019, 4:19 PM
  • Correct indentation
meven added a comment.Sep 1 2019, 4:26 PM

Looks good, just two small things.

src/panels/information/informationpanelcontent.cpp
244

A small optimization : invert the two condition, isAnimatedImage has already been computed and if false, mimeType.startsWith(QLatin1String("video/")) won't be run at all.

249

You can add an else if, animatedMimeTypes and usePhonon can't be true at the same time.
Let's make it more explicit.

iasensio updated this revision to Diff 65142.Sep 1 2019, 4:58 PM
iasensio marked an inline comment as done.
  • Small optimization
iasensio added inline comments.Sep 1 2019, 4:58 PM
src/panels/information/informationpanelcontent.cpp
249

It is mutually exclusive with usePhonon but not with the else clause afterwards. I could move this block to there, if that makes it more clear.

meven added inline comments.Sep 1 2019, 5:59 PM
src/panels/information/informationpanelcontent.cpp
249

No the next else after if (usePhonon) { if for if (InformationPanelSettings::previewsShown()) {.
The block if (usePhonon) { has no else block.

iasensio added inline comments.Sep 1 2019, 7:33 PM
src/panels/information/informationpanelcontent.cpp
249

Sorry, but I don't get it. I mean the else in line 278 (270 before the patch). I hope to have rebased correctly.

meven added inline comments.Sep 10 2019, 11:05 PM
src/panels/information/informationpanelcontent.cpp
249

Sorry my previous comment was not very clear.

Currently you have :

if (isAnimatedImaget) {}
if (isPhonon){}

I suggest to have

if (isAnimatedImaget) {}
else if (isPhonon){}

If you look at the code carefully you will see if (isPhonon){} has no else block.
The following else (line 278 / 286) is the else block of if (InformationPanelSettings::previewsShown()) {

I think we are having a misunderstanding, maybe due to the unlucky coincidence of line 278 having a different else on different branches 😃

This is an screenshot on current master code, without this patch. I'd like for the animatedImage to follow also the path on line 271.

Ugh, those kinds of if/else blocks are always confusing. I would (in another patch) re-arrange it so everything currently in the else block comes first (if (!usePhonon) { blabla) to get that small bit out of the way first so the more complex logic can come next without an else block after it.

meven added a comment.Sep 12 2019, 1:35 PM

This is an screenshot on current master code, without this patch. I'd like for the animatedImage to follow also the path on line 271.

Thank you for your patience, I was wrong. I got confused by the way phabricator collapse code, my bad.
So in the end the if (isAnimatedImage) { block could go to the else branch of if (usePhonon)

I have tested it. It works nicely.

Please wait a bit before merging so that @elvisangelaccio can have a look.
If he does not have time I will eventually accept it as is.

iasensio updated this revision to Diff 66005.Sep 13 2019, 4:40 PM
  • Move isAnimatedImage block for clarity

Ugh, those kinds of if/else blocks are always confusing. I would (in another patch) re-arrange it so everything currently in the else block comes first (if (!usePhonon) { blabla) to get that small bit out of the way first so the more complex logic can come next without an else block after it.

Sure! This method is loudly calling for refactoring, and I think @meven has that also in mind.
As for this patch, I moved the lines to the isPhonon:else block as suggested, which in fact helps to clarify the structure.

For later work on this code, I'm looking forward to your refactoring which, I think, will simplify the code readibility a lot, and mainly reduce the bug-prone capability of the current one.
(i.e. if you look carefully, there can also be some funny issues when moving between non-baloo searches ('isSearchUrl') and phonon thumbnails)

src/panels/information/informationpanelcontent.cpp
175

Please remove this comment, as D23668 will make it redundant.

224

Unrelated whitespace change

360

Then this is an unrelated change, I'll fix in on master.

src/panels/information/pixmapviewer.cpp
26

Please keep the list of includes sorted.

182–189

Is it really necessary to delete the QMovie instance and create another one every time?

191–196

I'd try to avoid to hardcode the list of supported mimetypes here. QMovie has QMovie::supportedFormats() which tells us the list of supported image formats.

We could create a QImageReader of the selected image and then check its QImageReader::format().

iasensio updated this revision to Diff 66128.Sep 15 2019, 3:07 PM
iasensio marked 5 inline comments as done.
  • Remove unrelated changes
  • Reuse QMovie instance
  • Check supported formats instead of hardcoded mimetypes

Thanks for the review and the hints @elvisangelaccio.
Those were some issues I tried to explore when starting the patch but didn't get to work, so I went the hardcoded way.
I hope it is in a better shape now.

elvisangelaccio accepted this revision.Sep 21 2019, 2:28 PM

Thanks, cool stuff :)

This revision is now accepted and ready to land.Sep 21 2019, 2:28 PM
This revision was automatically updated to reflect the committed changes.