Bug 155479 - MCGR: clumsy export of linear gradient to SVG
Summary: MCGR: clumsy export of linear gradient to SVG
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Impress (show other bugs)
Version:
(earliest affected)
7.6.0.0 alpha1+
Hardware: x86-64 (AMD64) Windows (All)
: medium normal
Assignee: Armin Le Grand
URL:
Whiteboard: target:24.2.0 target:7.6.0.0.beta2
Keywords: bibisected, bisected, regression
Depends on:
Blocks: SVG-Save
  Show dependency treegraph
 
Reported: 2023-05-24 23:28 UTC by Regina Henschel
Modified: 2023-06-28 11:11 UTC (History)
4 users (show)

See Also:
Crash report or crash signature:


Attachments
Shape with linear gradient (15.12 KB, application/zip)
2023-05-24 23:28 UTC, Regina Henschel
Details
A try to get ColorStops added to Metafile (22.03 KB, patch)
2023-06-02 10:34 UTC, Armin Le Grand
Details
Shape with no transparency and with transparency gradient (19.54 KB, application/vnd.oasis.opendocument.graphics)
2023-06-09 22:18 UTC, Regina Henschel
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Regina Henschel 2023-05-24 23:28:11 UTC
Created attachment 187486 [details]
Shape with linear gradient

Open attached document. Select the shape. File > Export. Select SVG. Mark 'Selection' and export.

Do this with a build before MCGR, e.g. from January, and do this with a current master. Compare the resulting SVG markup.

LO from January exports the gradient nicely as
      <linearGradient id="gradient1" x1="1177" y1="3363" x2="22596" y2="3363" gradientUnits="userSpaceOnUse">
       <stop offset="0.3" style="stop-color:rgb(255,255,0)"/>
       <stop offset="1" style="stop-color:rgb(0,112,192)"/>
      </linearGradient>

whereas current master produces a group of 90 <path>-elements.

The gradients in SVG are able to render our linear, axial and radial gradients. For these gradients the export should use the corresponding SVG-gradients.
Comment 1 m_a_riosv 2023-05-25 01:17:12 UTC
It's fine with
Version: 7.5.3.2 (X86_64) / LibreOffice Community
Build ID: 9f56dff12ba03b9acd7730a5a481eea045e468f3
CPU threads: 16; OS: Windows 10.0 Build 22621; UI render: Skia/Raster; VCL: win
Locale: es-ES (es_ES); UI: en-US Calc: CL threaded

Show the issue with
Version: 7.6.0.0.alpha1+ (X86_64) / LibreOffice Community
Build ID: f3aab159f1c1e00c25e6b4ca1e50813bc343f4f3
CPU threads: 16; OS: Windows 10.0 Build 22621; UI render: Skia/Raster; VCL: win
Locale: es-ES (es_ES); UI: en-US Calc: CL threaded
Version: 7.6.0.0.alpha0+ (X86_64) / LibreOffice Community
Build ID: 006b35d50024b1932d84380b5d2fec1f7066bccd
CPU threads: 16; OS: Windows 10.0 Build 22621; UI render: default; VCL: win
Locale: es-ES (es_ES); UI: en-US Calc: CL threaded

I can't find where old masters are. (Not in https://dev-builds.libreoffice.org/daily/master/)
Comment 2 Julien Nabet 2023-05-25 08:13:28 UTC
Armin: thought you might be interested in this one.
Comment 3 Gabor Kelemen (allotropia) 2023-05-25 12:57:16 UTC
Started with 

https://git.libreoffice.org/core/+/7c132fd14f7955e2bfbb600b78e939b9eae6a870

author	Armin Le Grand (allotropia) <armin.le.grand.extern@allotropia.de>	Tue Feb 28 11:32:43 2023 +0100
committer	Armin Le Grand <Armin.Le.Grand@me.com>	Wed Mar 01 09:18:07 2023 +0000

MCGR: support ColorSteps in emf export
Comment 4 Armin Le Grand 2023-05-31 11:00:19 UTC
Yes, I confirm that.

The SVG export is not wrong (it is correct), but not 'smart'. It does not use a SVG gradient.

Reason for that is that the SVG export is based on Metafile and Metafile can not really represent Gradients well, especially not after/with MCGR changes. It will be possible to e.g. fix this specific error by using 'border-detection' and then fallback to adding the adapted gradient to metafile. It will *not* be possible with metafiles to extend to 'real' MultiColor GradientStops.

That would need a re-design of the SVG export to be based on Primitives, alias PrimitiveRenderer. This is too much work to fix this bug, additionally since it's a shame to no longer have a svg gradient, but it still looks correct as SVG -> low priority from my POV, time would be better inversted in SVG redesign as mentioned.

Not sure what to do though here...
Comment 5 Regina Henschel 2023-05-31 12:04:39 UTC
Are ODF SVG Gradients on the ESC list for proposed TDF tenders? If yes, postpone the decision until ESC has voted. (I have no access to the list, it is not public.)
Comment 6 Armin Le Grand 2023-06-02 10:34:19 UTC
Created attachment 187662 [details]
A try to get ColorStops added to Metafile
Comment 7 Armin Le Grand 2023-06-02 10:40:57 UTC
Tried to get ColorStops added to Metafile, so we *could* use them at SVG export. Besides that it seems that "XPATHFILL_SEQ_BEGIN" and "XGRAD_SEQ_BEGIN" are somehow mixed-up nowadays in MetaCommentAction (which I found a way to compensate) for some reason (which I could not detect) ReadSvtGraphicFill does not read the BColorStops I wrote in WriteSvtGraphicFill ?!?
Anyways, using different actions in metafile is dangerous and I could not guarentee to trigger no side effects when doing it in this way, so I will not continue this for now.
To reliably solve this conversion/redesign of SVG based on UNO_API (using model data) or Primitives (using geometry data, should be good for SVG export I guess) would be needed.
Comment 8 Armin Le Grand 2023-06-02 11:23:53 UTC
Seems indeed an error in SvStream (?). With the patch applied I get at

        rIStm.ReadUInt16( nTmp );

in vcl/source/gdi/graphictools.cxx:304 a '0' instead of the correct number. I checked the stream pos, seems correct. Debugging into the ReadUInt16 shows that in tools/source/stream/stream.cxx:812 the test

    if (good())

indeed creates FALSE (?!?) where in 'n' at that moment we have the correct number.
That may have to do with

    aSerializer.readGraphic(rClass.maFillGraphic);

in graphictools.cxx:300 which *seems* to try to read a graphic and resets the ReadPos && error states at the stream -> maybe that goes WRONG???
Comment 9 Regina Henschel 2023-06-06 15:24:23 UTC
With https://gerrit.libreoffice.org/c/core/+/152623 the gradient is simple linearGradient. But that patch is not ready.
Comment 10 Armin Le Grand 2023-06-07 13:23:57 UTC
Okay, looks pretty good. One remaining problem that would be a regression from LO75 is:

Export to SVG of objects with TransparenceGradient (TG) *if* TRUE == TG->cannotBeHandledByVCL(). Independent of object's fill.

In the example this happens since TG has three steps and cannot be represented by vcl::Gradient, thus a replacement BMP gets created for metafile recording.
This is @drawinglayer/source/processor2d/vclmetafileprocessor2d.cxx:2367 in VclMetafileProcessor2D::processTransparencePrimitive2D and a fix for tdf#155437.

That makes it complicated. I looked into it to the point where the *gradient* part of the MetaFloatTransparentAction could be transfered similar to what we do here already, but in SvmWriter::FloatTransparentHandler and SvmReader::FloatTransparentHandler.
NOTE: The *content* held at that action as metafile seems to get converted to BMP in many use cases for MetaActionType::FLOATTRANSPARENT, so I am not sure how useful that would be besides being able to write that TG to SVG directly.

I assume opening another separate bug for that one might be better...?
Comment 11 Armin Le Grand 2023-06-07 14:04:46 UTC
I detected another caveat: With this fix the metafile contains
- BGRAD_SEQ_BEGIN
- MetaActionType::GRADIENTEX
- all simple-color-filled gradient steps from decomposition
- BGRAD_SEQ_END

This is handled correct by filter/source/svg/svgwriter.cxx:3350 since it *knows* BGRAD_SEQ_BEGIN/END, so only MetaActionType::GRADIENTEX is used.

All other usages of MetaActionType::COMMENT (51 hits, not all case ..., but MANY) do not know that and will potentially handle that gradient *twice*.

That means: We cannot do it like that. That would require to adapt all (potentially 51) places where this may now be used. This is EXACTLY why I did primitives and a decomposition, so only *one* place would have to be changed - ARGH!

This means we *need to know* that SVG export is in progress when adding BGRAD_SEQ_BEGIN to the metafile.
I checked, but I see no info @metafile or @OutputDevcie...?

(there is a m_bUseCanvas @metafile, we may have to something similar. The place to set a flag @metafile would probably be SVGFilter::implCreateObjectsFromShape...?)
Comment 12 Armin Le Grand 2023-06-09 08:15:21 UTC
Have now added code to do on SVG export only. Also added code to get the TransparencyGradients handled in a similar way, that will solvethe problem described in comment 10.
Comment 13 Commit Notification 2023-06-09 08:17:36 UTC
Armin Le Grand (allotropia) committed a patch related to this issue.
It has been pushed to "master":

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

MCGR: tdf#155479 repair gradient SVG export for MCGR

It will be available in 24.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 14 Regina Henschel 2023-06-09 22:10:47 UTC
It is not resolved. I still get no <linearGradient> element if a transparency gradient is present. Example file will be attached.

I think, we should stop to try to solve this by tweaking the use of metafile. Export of selected shapes to SVG needs an own solution separated from rendering. SVG and ODF are both xml and have a lot of elements in common. The export to SVG should be similar to the export to ODF.
Comment 15 Regina Henschel 2023-06-09 22:18:34 UTC
Created attachment 187813 [details]
Shape with no transparency and with transparency gradient

Export each of the two shapes to SVG. For the opaque shape you get a nice <linearGradient> element for the color gradient. For the shape with transparency gradient you get a <linearGradient> element for the alpha mask (that is good), but no longer a color gradient for the shape fill.

I made sure that 'step count' is really set to 'automatic'.
Comment 16 Commit Notification 2023-06-13 21:21:15 UTC
Armin Le Grand (allotropia) committed a patch related to this issue.
It has been pushed to "libreoffice-7-6":

https://git.libreoffice.org/core/commit/9b8c21acd31f08a0c3f8d88ddac57c80ef5997a1

MCGR: tdf#155479 repair gradient SVG export for MCGR

It will be available in 7.6.0.0.beta2.

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 Armin Le Grand 2023-06-27 11:45:06 UTC
Yupp, that's correct. Thanks for finding it!
I fixed the case with non-vcl-renderable gradient, the one with non-vcl-renderable transparenceGradient and the combination. What happens with the example is that the transparenceGradient *is* vcl-renderable, so the sub-content gets not created correctly.
All in all it just gets down to always propagate the isSVG flag for metafiles -> in *any* case impDumpToMetaFile is used and a new metafile gets created (aContentMetafile here) that flag needs to be propagated to that due to the subContent again may have any combinations of gradients/transparenceGradients that are non-vcl-renderable.
Adapting and checking...
Comment 18 Commit Notification 2023-06-27 16:37:43 UTC
Armin Le Grand (allotropia) committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/3b06c1835e9fcbbcdcd6ce2b207301f4f8bb6388

MCGR: tdf#155479 always propagate SVG-flag for sub-content metafiles

It will be available in 24.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 19 Commit Notification 2023-06-28 11:11:00 UTC
Armin Le Grand (allotropia) committed a patch related to this issue.
It has been pushed to "libreoffice-7-6":

https://git.libreoffice.org/core/commit/170a3cfa47e2e4ec83ec91d654601894e246b9d2

MCGR: tdf#155479 always propagate SVG-flag for sub-content metafiles

It will be available in 7.6.0.0.beta2.

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.