Bug 446320

Summary: Crash when rendering to GIF
Product: [Applications] krita Reporter: Halla Rempt <halla>
Component: AnimationAssignee: Eoin O'Neill <eoinoneill1991>
Status: RESOLVED FIXED    
Severity: normal CC: ahab.greybeard, eoinoneill1991, knowzero
Priority: NOR Keywords: regression
Version First Reported In: git master (please specify the git hash!)   
Target Milestone: ---   
Platform: Other   
OS: Linux   
Latest Commit: Version Fixed/Implemented In:
Sentry Crash Report:

Description Halla Rempt 2021-12-01 11:54:43 UTC
a81d83d340ea0fa0fcee652963faf262b92a07c3

After rendering to GIF, krita asserts: 

krita(2144656)/(default) kis_assert_common: ASSERT (krita): "m_process == nullptr" in file /home/halla/dev/krita/libs/ui/animation/KisFFMpegWrapper.cpp, line 47

See https://krita-artists.org/t/krita-next-crashes-when-rendering-animations-as-gif/32651
Comment 1 Know Zero 2021-12-01 20:16:35 UTC
(In reply to Halla Rempt from comment #0)
> a81d83d340ea0fa0fcee652963faf262b92a07c3
> 
> After rendering to GIF, krita asserts: 
> 
> krita(2144656)/(default) kis_assert_common: ASSERT (krita): "m_process ==
> nullptr" in file
> /home/halla/dev/krita/libs/ui/animation/KisFFMpegWrapper.cpp, line 47
> 
> See
> https://krita-artists.org/t/krita-next-crashes-when-rendering-animations-as-
> gif/32651

When gif is created, it is done in 2 parts. First to generate the palette. Then using the palette to generate the final file. thus it fails the m_process == nullptr as the pointer still holds the last process.

Adding m_process.reset() 

after 

158     if (processResults->finish == true) {

should fix it.
Comment 2 Ahab Greybeard 2021-12-01 20:26:06 UTC
I can confirm this for the 5.0.0-beta3 appimage onwards.
It does not happen in the 5.0.0-beta2 appimage hence regression.

I only saw an Assert once and that was in the log with beta3:
ASSERT (krita): "m_process == nullptr" in file /home/appimage/workspace/Krita_Release_Appimage_Build/krita/libs/ui/animation/KisFFMpegWrapper.cpp, line 47

Also, the time taken to produce intermediate .png files and the time taken to render is at least 3 times slower than for beta-2 and earlier versions.
Comment 3 Know Zero 2021-12-01 21:08:05 UTC
(In reply to Know Zero from comment #1)
> (In reply to Halla Rempt from comment #0)
> > a81d83d340ea0fa0fcee652963faf262b92a07c3
> > 
> > After rendering to GIF, krita asserts: 
> > 
> > krita(2144656)/(default) kis_assert_common: ASSERT (krita): "m_process ==
> > nullptr" in file
> > /home/halla/dev/krita/libs/ui/animation/KisFFMpegWrapper.cpp, line 47
> > 
> > See
> > https://krita-artists.org/t/krita-next-crashes-when-rendering-animations-as-
> > gif/32651
> 
> When gif is created, it is done in 2 parts. First to generate the palette.
> Then using the palette to generate the final file. thus it fails the
> m_process == nullptr as the pointer still holds the last process.
> 
> Adding m_process.reset() 
> 
> after 
> 
> 158     if (processResults->finish == true) {
> 
> should fix it.

Looking at:

https://bugs.kde.org/show_bug.cgi?id=442578

It seems the goal was to prevent manually doing the cleanup. If that is the case, other options include creating a separate FFMpegWrapper to run the palette, or merge the palettegen and paletteuse into 1 command.
Comment 4 Eoin O'Neill 2021-12-01 21:40:11 UTC
(In reply to Know Zero from comment #3)
> (In reply to Know Zero from comment #1)
> > (In reply to Halla Rempt from comment #0)
> > > a81d83d340ea0fa0fcee652963faf262b92a07c3
> > > 
> > > After rendering to GIF, krita asserts: 
> > > 
> > > krita(2144656)/(default) kis_assert_common: ASSERT (krita): "m_process ==
> > > nullptr" in file
> > > /home/halla/dev/krita/libs/ui/animation/KisFFMpegWrapper.cpp, line 47
> > > 
> > > See
> > > https://krita-artists.org/t/krita-next-crashes-when-rendering-animations-as-
> > > gif/32651
> > 
> > When gif is created, it is done in 2 parts. First to generate the palette.
> > Then using the palette to generate the final file. thus it fails the
> > m_process == nullptr as the pointer still holds the last process.
> > 
> > Adding m_process.reset() 
> > 
> > after 
> > 
> > 158     if (processResults->finish == true) {
> > 
> > should fix it.
> 
> Looking at:
> 
> https://bugs.kde.org/show_bug.cgi?id=442578
> 
> It seems the goal was to prevent manually doing the cleanup. If that is the
> case, other options include creating a separate FFMpegWrapper to run the
> palette, or merge the palettegen and paletteuse into 1 command.

I'm on it.
Comment 5 Eoin O'Neill 2021-12-01 22:00:05 UTC
Git commit fd48ced97aba7d821bd2ddecc3b1c70d1fbff2d8 by Eoin O'Neill.
Committed on 01/12/2021 at 21:58.
Pushed by eoinoneill into branch 'master'.

Fix assert trigger when using FFMPEGWrapper for gif format.

Trying to re-use the process without proper termination resulted
in assert. It's good that we caught it, but it's now fixed...

M  +5    -3    libs/ui/animation/KisFFMpegWrapper.cpp
M  +1    -1    libs/ui/animation/KisFFMpegWrapper.h
M  +3    -0    libs/ui/animation/KisVideoSaver.cpp
M  +1    -1    plugins/dockers/recorder/recorder_export.cpp

https://invent.kde.org/graphics/krita/commit/fd48ced97aba7d821bd2ddecc3b1c70d1fbff2d8
Comment 6 Eoin O'Neill 2021-12-01 22:00:56 UTC
Git commit 95b4dc84ef01598703e9893e2f4aec45b7da5bc2 by Eoin O'Neill.
Committed on 01/12/2021 at 22:00.
Pushed by eoinoneill into branch 'krita/5.0'.

Fix assert trigger when using FFMPEGWrapper for gif format.

Trying to re-use the process without proper termination resulted
in assert. It's good that we caught it, but it's now fixed...
(cherry picked from commit fd48ced97aba7d821bd2ddecc3b1c70d1fbff2d8)

M  +5    -3    libs/ui/animation/KisFFMpegWrapper.cpp
M  +1    -1    libs/ui/animation/KisFFMpegWrapper.h
M  +3    -0    libs/ui/animation/KisVideoSaver.cpp
M  +1    -1    plugins/dockers/recorder/recorder_export.cpp

https://invent.kde.org/graphics/krita/commit/95b4dc84ef01598703e9893e2f4aec45b7da5bc2