Bug 161724 - FILEOPEN PPTX: image completely disappears, other quite off (zoomed in?)
Summary: FILEOPEN PPTX: image completely disappears, other quite off (zoomed in?)
Status: VERIFIED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Impress (show other bugs)
Version:
(earliest affected)
24.8.0.0 beta1+
Hardware: All All
: high major
Assignee: Xisco Faulí
URL:
Whiteboard: target:25.2.0 target:24.8.1 target:24...
Keywords: bibisected, bisected, regression
: 162145 162213 (view as bug list)
Depends on:
Blocks: PPTX-Images
  Show dependency treegraph
 
Reported: 2024-06-21 09:39 UTC by Gerald Pfeifer
Modified: 2024-09-25 00:42 UTC (History)
8 users (show)

See Also:
Crash report or crash signature:


Attachments
Sample slide (PPTX) (462.19 KB, application/vnd.openxmlformats-officedocument.presentationml.presentation)
2024-06-21 09:39 UTC, Gerald Pfeifer
Details
Visual comparison: PowerPoint (top) - Impress (bottom) (309.12 KB, image/png)
2024-06-21 09:40 UTC, Gerald Pfeifer
Details
Sample document with right and wrong behaviour (158.55 KB, application/vnd.openxmlformats-officedocument.presentationml.presentation)
2024-09-04 14:53 UTC, Xisco Faulí
Details
Sample document with right and wrong behaviour (155.45 KB, application/vnd.openxmlformats-officedocument.presentationml.presentation)
2024-09-04 15:14 UTC, Xisco Faulí
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Gerald Pfeifer 2024-06-21 09:39:49 UTC
Created attachment 194876 [details]
Sample slide (PPTX)

This is a regression in

   Version: 24.8.0.0.beta1+ (X86_64) / LibreOffice Community
   Build ID: 0f06d45cd69b17583ca5e601ecb8fb3342f37501
   CPU threads: 12; OS: Linux 6.9; UI render: default; VCL: gtk3
   Locale: en-US (en_US.UTF-8); UI: en-US

and

   Version: 25.2.0.0.alpha0+ (X86_64) / LibreOffice Community
   Build ID: 151d997365f7bf271d63af535d29a9c3439c6d46
   CPU threads: 12; OS: Linux 6.9; UI render: default; VCL: gtk3
   Locale: en-US (en_US.UTF-8); UI: en-US


Still fine:

   Version: 24.2.4.0.0+ (X86_64) / LibreOffice Community
   Build ID: 2dd760424554d597eb93fb6bc96ffddc9c5f1b18
   CPU threads: 12; OS: Linux 6.9; UI render: default; VCL: gtk3
   Locale: en-US (en_US.UTF-8); UI: en-US


(Personal note: bsc#1170874 has the full document.)
Comment 1 Gerald Pfeifer 2024-06-21 09:40:27 UTC
Created attachment 194877 [details]
Visual comparison: PowerPoint (top) - Impress (bottom)
Comment 2 Telesto 2024-06-21 19:36:53 UTC
Confirm
Version: 25.2.0.0.alpha0+ (X86_64) / LibreOffice Community
Build ID: 151d997365f7bf271d63af535d29a9c3439c6d46
CPU threads: 8; OS: macOS 14.3; UI render: Skia/Raster; VCL: osx
Locale: nl-NL (nl_NL.UTF-8); UI: en-US
Calc: threaded
Comment 3 Aron Budea 2024-06-28 23:06:58 UTC
Regression is from the following commit, bibisected using repo bibisect-win64-24.8. Adding CC: to Attila Szűcs.

https://git.libreoffice.org/core/+/c30c1d12f283e75fdcc5bb508a79a9d33a431d28%5E!
Author:     Attila Szűcs <attila.szucs@collabora.com>
AuthorDate: Mon Jun 3 22:08:42 2024 +0200
Commit:     Caolán McNamara <caolan.mcnamara@collabora.com>
CommitDate: Thu Jun 6 10:03:14 2024 +0200

tdf#153008 svx: impl crop for stretched bitmap fill
Comment 4 Gerald Pfeifer 2024-06-29 08:48:11 UTC
Adding Caolán, as committer of the patch identified to trigger the
regression.
Comment 5 Xisco Faulí 2024-09-04 14:31:54 UTC
@Miklos, I thought you might be interested in this issue
Comment 6 Xisco Faulí 2024-09-04 14:53:40 UTC
Created attachment 196230 [details]
Sample document with right and wrong behaviour

It seems if the same Offset (left, right, top and bottom ) is set, then it works correctly. Otherwise it fails
Comment 7 Xisco Faulí 2024-09-04 14:54:00 UTC
*** Bug 162213 has been marked as a duplicate of this bug. ***
Comment 8 Xisco Faulí 2024-09-04 15:14:28 UTC
Created attachment 196234 [details]
Sample document with right and wrong behaviour
Comment 9 Xisco Faulí 2024-09-04 15:18:22 UTC
When all offsets values are the same then getStretch() returns false. So when it's true, then the calculation is done incorrectly in drawinglayer/source/attribute/sdrfillgraphicattribute.cxx.My feeling is that https://git.libreoffice.org/core/+/c30c1d12f283e75fdcc5bb508a79a9d33a431d28%5E%21 is not completed and works in the attachment from bug 153008 but not with other cases
Comment 10 Miklos Vajna 2024-09-05 06:15:22 UTC
(In reply to Xisco Faulí from comment #6)
> It seems if the same Offset (left, right, top and bottom ) is set, then it
> works correctly. Otherwise it fails

Maybe then it would make sense to restrict the new behavior to just this case for now?

Unless somebody has the time to debug & fix the root cause. Thanks.
Comment 11 Xisco Faulí 2024-09-05 12:39:28 UTC
*** Bug 162145 has been marked as a duplicate of this bug. ***
Comment 12 Xisco Faulí 2024-09-05 12:50:38 UTC
Reverting it for now: https://gerrit.libreoffice.org/c/core/+/172913
See my comment in https://bugs.documentfoundation.org/show_bug.cgi?id=153008#c16
Comment 13 Xisco Faulí 2024-09-05 12:58:49 UTC
attachment 184636 [details] from bug 153008 is another document affected.
I've also created attachment 196251 [details] from bug 95165 with 10 different images where the issue is affected in some of them
Comment 14 Commit Notification 2024-09-05 14:18:25 UTC
Xisco Fauli committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/295e1039649de030babf3ac9235cc80f9b9ca33c

tdf#161724: Revert "tdf#153008 svx: impl crop for stretched bitmap fill"

It will be available in 25.2.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 15 Gerald Pfeifer 2024-09-05 19:19:03 UTC
Thank you, Xisco! Nice to see this regression addressed.


One question: Do we have testcases that'll trigger were anyone to
revert the revert (i.e., reapply the original patch)?

Having a safety net to catch this regression automatically would be
great.
Comment 16 Commit Notification 2024-09-06 07:45:30 UTC
Xisco Fauli committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/8ffcd108d6eb233130e7c740bea235b923a4883f

tdf#161724: svx_unit: Add unittest

It will be available in 25.2.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 17 Xisco Faulí 2024-09-06 07:52:49 UTC
(In reply to Gerald Pfeifer from comment #15)
> Thank you, Xisco! Nice to see this regression addressed.
> 
> 
> One question: Do we have testcases that'll trigger were anyone to
> revert the revert (i.e., reapply the original patch)?
> 
> Having a safety net to catch this regression automatically would be
> great.

See comment 16 ;-)
Comment 18 Commit Notification 2024-09-06 10:43:16 UTC
Xisco Fauli committed a patch related to this issue.
It has been pushed to "libreoffice-24-8-1":

https://git.libreoffice.org/core/commit/f8d50a8fa556c5bfb0d638117b8b0deafd433af5

tdf#161724: Revert "tdf#153008 svx: impl crop for stretched bitmap fill"

It will be available in 24.8.1.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 19 Commit Notification 2024-09-06 11:19:26 UTC
Xisco Fauli committed a patch related to this issue.
It has been pushed to "libreoffice-24-8":

https://git.libreoffice.org/core/commit/8fdb6449f9f991ae1eae4a5742081986014de7cd

tdf#161724: Revert "tdf#153008 svx: impl crop for stretched bitmap fill"

It will be available in 24.8.2.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 20 Gerald Pfeifer 2024-09-12 12:07:27 UTC
Verified as fixed for example with today's daily build

  Version: 25.2.0.0.alpha0+ (X86_64) / LibreOffice Community
  Build ID: e0a5b29f653a314727e23489a3e98daff8386ff6
  CPU threads: 12; OS: Linux 6.10; UI render: default; VCL: gtk3
  Locale: en-US (en_US.UTF-8); UI: en-US 

Thank you for tackling this, Xisco!
Comment 21 Attila Szűcs 2024-09-25 00:42:08 UTC
I write it here too

Sorry for the regression, I really worked only on the 2. bug of that ticket... and that bug should be a separate bug from the 1. one..
i debugged what happening and it seems a bit worse i thought...

the good vs bad xml:
bad:
<p:pic>
	<p:blipFill rotWithShape="1">
		<a:srcRect l="44198" r="1196" b="-1"/>
		<a:stretch/>

good:
<p:sp>
	<p:spPr>
		<a:blipFill>
			<a:srcRect/>
			<a:stretch>
				<a:fillRect l="20048" t="23753" r="-40072" b="-2983"/>
			</a:stretch>

This is the 2 very different case: 
<a:srcRect ..>
<a:fillRect ..>

And they both sent into PROP_GraphicCrop!! (it is not my patch it was like this from long ago) .. althought they are different kind of crops.. and i did not seen any property around it that would tell where it came from.

In my patch, i implemented a missing crop like calculation ... but unfortunatelly it now executed on <a:srcRect ..> case as well, where it should not.

So i think to fix it..
a) the 2 different crop like things should be separated.. they should be stored in 2 different property..
or
b) at least, it would be good to have an additional property that show which crop is stored in PROP_GraphicCrop.. so i could choose to do my calculation or not...