Bug 409414 - pixelDataAtTime seems to empty the last frame read.
Summary: pixelDataAtTime seems to empty the last frame read.
Status: RESOLVED FIXED
Alias: None
Product: krita
Classification: Applications
Component: Animation (show other bugs)
Version: git master (please specify the git hash!)
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: Krita Bugs
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-07-02 12:51 UTC by wolthera
Modified: 2020-08-26 08:23 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed In:
Sentry Crash Report:


Attachments
Python script that shows the inappropriate behaviour. (373 bytes, text/x-python)
2019-07-02 12:51 UTC, wolthera
Details

Note You need to log in before you can comment on or make changes to this bug.
Description wolthera 2019-07-02 12:51:57 UTC
Created attachment 121286 [details]
Python script that shows the inappropriate behaviour.

SUMMARY
When trying to make a spritesheet python script, pixelDataAtTime is called to get a bytearray that can then be copied to a target.


STEPS TO REPRODUCE
1. Take a small animation.
2. Run the attached python script while having an animated layer active.
3. Flip between layers, and observe how the last keyframe of the layer the script is run on is now empty.

OBSERVED RESULT
the last frame is emptied.

EXPECTED RESULT
just reading the data of the last frame shouldn't do anything to it?

System info:
-------------------
Krita

 Version: 4.3.0-prealpha (git d649293)
 Languages: en_US, en_GB, nl
 Hidpi: true

Qt

  Version (compiled): 5.12.3
  Version (loaded): 5.12.3

OS Information

  Build ABI: x86_64-little_endian-lp64
  Build CPU: x86_64
  CPU: x86_64
  Kernel Type: linux
  Kernel Version: 4.15.0-52-generic
  Pretty Productname: KDE neon User Edition 5.16
  Product Type: neon
  Product Version: 18.04
Comment 1 Halla Rempt 2020-05-19 12:46:48 UTC
* It's the doc.refreshProjection() line that actually kills the last frame, it seems. 
* Pressing undo twice restores the last frame
Comment 2 Halla Rempt 2020-05-19 12:52:57 UTC
Pressing undo gives a safe assert:

from krita import *

SAFE ASSERT (krita): "changeList.memento == memento" in file /home/boud/dev/4.3/libs/image/tiles3/kis_memento_manager.cc, line 278
^[[2;5~SAFE ASSERT (krita): "changeList.memento == memento" in file /home/boud/dev/4.3/libs/image/tiles3/kis_memento_manager.cc, line 335
SAFE ASSERT (krita): "changeList.memento == memento" in file /home/boud/dev/4.3/libs/image/tiles3/kis_memento_manager.cc, line 278
Comment 3 Halla Rempt 2020-05-19 12:58:02 UTC
This isn't a bug in libkis; there is some very weird interaction between accessing frames and calling for projection updates, which in itself invalidates the animation cache. It's just that with a script, we can actually expose the issue.
Comment 4 Eoin O'Neill 2020-08-24 19:46:11 UTC
Note: What's really strange here is that this isn't simply a caching issue, but actually and issue with the update walker.

Sometimes, the last frame is made fully null, but if your current time is in the middle frame, it will sometimes shuffle the order of the keyframes entirely. This is pretty dangerous..

I'll do more investigation.
Comment 5 Eoin O'Neill 2020-08-25 04:31:06 UTC
Git commit 4a5eb9d5378ac6041dcaa54ed821979234345f22 by Eoin O'Neill.
Committed on 25/08/2020 at 04:30.
Pushed by eoinoneill into branch 'master'.

`pixelDataAtTime` Implementation Correction

An incorrect call to KisRasterKeyframeChannel::fetchFrame was causing undefined behavior
when reading from a specific frame. Instead of passing in a write-target paint device,
it was using the existing node's paint device which was causing the frame at current time
to be overwritten by essentially arbitrary frame data.

This user-error is easy to make with the original method name ('fetchFrame' doesn't sound
like a method that writes to a paint device, which made argument 2 somewhat confusing
from an outsiders perspective) and has already been addressed in our Refactor, but it's
probably worth fixing this first in an isolated commit.

Additional Note: the paint device being made to write to is using a snapshot copy to create
the temporary device before feeding into the fetchFrame method. Since we basically just
want an empty 'sheet' to write to, the contents of the frame should be mostly arbitrary before
writing.

M  +1    -1    libs/libkis/Node.cpp

https://invent.kde.org/graphics/krita/commit/4a5eb9d5378ac6041dcaa54ed821979234345f22
Comment 6 Halla Rempt 2020-08-26 08:23:44 UTC
Git commit 740a404770bb76ed57d21b376b0e9bbe2c15cb6a by Boudewijn Rempt, on behalf of Eoin O'Neill.
Committed on 26/08/2020 at 08:11.
Pushed by rempt into branch 'krita/4.3'.

`pixelDataAtTime` Implementation Correction

An incorrect call to KisRasterKeyframeChannel::fetchFrame was causing undefined behavior
when reading from a specific frame. Instead of passing in a write-target paint device,
it was using the existing node's paint device which was causing the frame at current time
to be overwritten by essentially arbitrary frame data.

This user-error is easy to make with the original method name ('fetchFrame' doesn't sound
like a method that writes to a paint device, which made argument 2 somewhat confusing
from an outsiders perspective) and has already been addressed in our Refactor, but it's
probably worth fixing this first in an isolated commit.

Additional Note: the paint device being made to write to is using a snapshot copy to create
the temporary device before feeding into the fetchFrame method. Since we basically just
want an empty 'sheet' to write to, the contents of the frame should be mostly arbitrary before
writing.
(cherry picked from commit 4a5eb9d5378ac6041dcaa54ed821979234345f22)

M  +1    -1    libs/libkis/Node.cpp

https://invent.kde.org/graphics/krita/commit/740a404770bb76ed57d21b376b0e9bbe2c15cb6a