OpenShot / openshot-qt

OpenShot Video Editor is an award-winning free and open-source video editor for Linux, Mac, and Windows, and is dedicated to delivering high quality video editing and animation solutions to the world.
http://www.openshot.org
Other
4.39k stars 549 forks source link

SVG files not rendered properly #1081

Closed N3WWN closed 5 years ago

N3WWN commented 7 years ago

I've seen this since one of the Openshot 2.3 versions (never used anything older than 2.3) and can confirm the issue is still present in 2.4.1 cuz I just used 2.4.1 to prepare the support files for this issue. This has always been on Fedora 25.

I use titles to overlay statistics on my videos. I like to have fun with them (sometimes) and wanted a creepy font with fun colors for a Halloween-themed video I was working on.

Starting with one of the stock titles and using the Advanced Editor (inkscape), I created a title using the Shlop font, made it green and turned the stroke red.

This file is provided in the attached zip archive: Stage 1 stats.svg

Directly using/importing this SVG file within Openshot results in very different rendering, most of which is off screen due to the text anchor point being honored, but the alignment is ignored.

In order to better see how Openshot is rendering the SVG file, I copied the file and manually edited it, adding an x-offset of -800 pixels.

This file is provided in the attached zip archive: Stage 1 stats_offset.svg

In order to use this SVG file, I had to convert it to a PNG on the command line and import the transparent image.

This file is provided in the attached zip archive: Stage 1 stats.png

The OSP file is included in the attached zip archive, too: SVG rendering issue - 171128.osp

An Openshot 2.4.1 rendered video that shows each of the 3 files and demonstrates that the issue is present in the final export (not just the preview) is available here: https://youtu.be/1R6uUPfotss

SVG rendering issue - 171128.zip

Please let me know if I can provide any additional information or help.

Thanks!

-Rich

DylanC commented 7 years ago

@N3WWN - Wow, this is an example of a really great bug report! O_O Thanks a million!

donkikhot commented 6 years ago

Hi i have this problem too OpenShot 1.4.3 (font Cyrillic old)

peanutbutterandcrackers commented 6 years ago

@donkikhot - Good sir, Openshot 1.x series is no longer under development. Perhaps you should try the 2.x series out.

@N3WWN - Wow. That's a really good report. :+1:

donkikhot commented 6 years ago

Ok, thanks.

peanutbutterandcrackers commented 6 years ago

@donkikhot - I will try to bring the developer's attention to this, too. Hopefully, this will be fixed soon. I hope you keep on supporting OpenShot and do report any issues as they arise. Thanks! :)

peanutbutterandcrackers commented 6 years ago

@DylanC - Can you confirm this, cap'ain? I am not that good with InkScape. Perhaps this ought to be put in the check list for v2.4.2?

gary9872 commented 6 years ago

Didn't work for me either

titles_model:INFO updating title model. exceptions:ERROR Unhandled Exception Traceback (most recent call last): File "/usr/lib/python3.6/site-packages/openshot_qt/windows/main_window.py", line 357, in actionEditTitle_trigger win = TitleEditor(file_path) File "/usr/lib/python3.6/site-packages/openshot_qt/windows/title_editor.py", line 120, in init self.load_svg_template() File "/usr/lib/python3.6/site-packages/openshot_qt/windows/title_editor.py", line 275, in load_svg_template self.update_font_color_button() File "/usr/lib/python3.6/site-packages/openshot_qt/windows/title_editor.py", line 372, in update_font_color_button s = node.attributes["style"].value File "/usr/lib/python3.6/xml/dom/minidom.py", line 552, in getitem return self._attrs[attname_or_tuple] KeyError: 'style'

peanutbutterandcrackers commented 6 years ago

Thank you for the confirmation. Are you using the latest daily build? Does it render incorrectly or are you unable to import the svg file at all? o.O

DylanC commented 6 years ago

@peanutbutterandcrackers - Maybe consider this for the checklist.

peanutbutterandcrackers commented 6 years ago

@DylanC - The checklist has been updated, Cap'ain!

N3WWN commented 6 years ago

FYI, I'm working on resolving this right now. It will hopefully be my next PR for this project.

peanutbutterandcrackers commented 6 years ago

Awesome! I am looking forward to the PR! :)

N3WWN commented 6 years ago

OMG! I can't believe I messed up the PR for this! I somehow PR'd zero changes back into my dev branch instead of here.

At any rate, would @peanutbutterandcrackers , @DylanC , @ferdnyc and/or @jonoomph take a look at my changes and do a quick sanity check on them:

https://github.com/OpenShot/libopenshot/compare/master...N3WWN:resvg_rendering

I'll remove the lines with my initials (RDA) before doing the actual PR. I use them as search anchors to jump around in files quickly.

There will need to a change to openshot_qt/installer/build-server.py and openshot_qt/freeze.py so that the resvg.so library is packaged with the AppImage builds. I'm not sure what else will need to be changed for other pre-packaged builds as I'm only ever using AppImage builds, even for my own local testing.

Basically, if the image file has an .svg extension, the rendering is performed by an external library (resvg.so) and the rendered image is saved to the homePath()/.openshot_qt/cache directory. resvg supports a "static SVG Full 1.1 subset", which is much more than QtSvg, which "officially supports only a tiny portion of the SVG Tiny 1.2 subset".

cmake/Modules/FindRESVG.cmake will try to find the resvg library and, if it is missing, libopenshot will be compiled and function exactly as it does now. This means that devs and power users can clone the libopenshot repo and compile it themselves in the presence of the resvg.so library to gain this feature, but it does not need to be compiled into the publicly released builds right away.

Feedback welcomed for this change before I submit the PR so I can fix any problems beforehand.

Thanks!

ferdnyc commented 6 years ago

Hmm, seems I can't comment on that branch as it's not a pull request. That's fine, here works.

I haven't tested the SVG changes yet, but I will try to. But if there are rendering issues with the current lib, and resvg fixes them, then changing over sounds sensible. Thanks for doing the work to find an alternative.

I'm a bit concerned that sudo dnf search resvg on my Fedora 27 box returns no results — is the library packaged for standard distros? How is it identified? The dependency change will have to be communicated to packagers so that they can update their builds.

That aside, one minor nit: I'd suggest submitting OpenShot/libopenshot@16b9cd949df393414c118136520b308b04505254 as a separate pull request from the rest. I'm curious to see how it affects the logging output, but on first glance it looks like a good change to me. If I'm interpreting correctly it's totally unrelated to the SVG rendering changes, though, yes? Better if each logical change stands alone in its own PR, and can be evaluated on its own merits.

ferdnyc commented 6 years ago

I subsequently (and accidentally) deleted the comment that referenced this issue, but I'm glad the link stayed here because I ended up tossing a bunch of inline comments at the commit, which can be seen by following the "ferdnyc referenced this issue..." link above. They don't show up inline if you follow @N3WWN 's link (though I now see they're collected at the bottom of the page), because I didn't have permission to add inline comments there. For some reason! (That reason being: Github is confusing.)

I looked into resvg a bit more. It's distributed under the Mozilla Public License 2.0, which is good because that's considered a GPL-compatible license and is permissible for packaging in Fedora and most likely other libre distros as well, unless the license user has explicitly chosen to make it incompatible (which it doesn't appear RazrFalcon has).

Surprisingly enough, though, it appears that nobody has even proposed its inclusion in the Fedora packaging system. ...Actually, I guess that's not so surprising when you consider that it's only even been available on Github since December 18. Yes, of last year. This thing is really new, and OpenShot would likely be the impetus for its inclusion in many distros. That's not necessarily a bad thing, assuming it's mature and stable enough to meet our needs, and using it solves problems.

I was hoping that perhaps the rendering issue was caused by older AppImage versions of QtSVG, but after building the Qt svgviewer example project under Qt 5.9.4 and using it to load both of the sample .svg files, it's clearly just doing things... wrong. So, QtSVG is definitely failing us here.

(It probably makes sense, for their purposes, that Qt is targeting SVG Tiny 1.2 with QtSVG, as that's explicitly "a profile of SVG intended for implementation on a range of devices, from cellphones and PDAs to laptop and desktop computers" and Qt is very focused on mobile-platform support these days. But it does mean that if we want to incorporate arbitrary files from a full-featured editor like Inkscape, it's probably not going to be up to snuff. QtSVG is meant, it seems, to work with SVG files which have been designed with the intent of using them in Qt applications, and therefore can avoid coloring outside the lines of SVG Tiny 1.2.)

ferdnyc commented 6 years ago

@peanutbutterandcrackers writes:

@DylanC - The checklist [for v2.4.2] has been updated, Cap'ain!

Checklist? There's a checklist?

Does this mythical checklist exist anywhere accessible to others? Seems like it could be a useful thing to have outward-facing.

I know Github has the Projects feature for basically this purpose, though I've never used it myself. There's a Projects tab in the repo here, though currently the only thing it contains is an "ad" for the feature and a link to that doc. So I'm assuming @jonoomph or someone would have to enable access, before it could be used for anything. Still, even a Github issue containing the list would be a way to connect issues to it, sort of like a tracking bug in bugzilla.

peanutbutterandcrackers commented 6 years ago

@ferdnyc - Here it is. It's just a humble little check-list. :)

@N3WWN - I'm afraid I won't be able to test this out as I am working from a friend's computer even right now. But I'll try to see if I can. However, with @ferdnyc here and @DylanC, I think you have plenty of good testers. But awesome, good sir! You're amazing!

ferdnyc commented 6 years ago

[Checklist] Neato.

Another question I can't believe I didn't think to ask until now: Do we know the status of resvg on Windows/MacOS? Has anyone (on their end, I mean) looked into building it for those platforms? If QtSVG is failing us, it's failing us on every platform, and whatever replacement we use should probably be used everywhere.

I have no idea where resvg stands on that front, I'm honestly asking. Hopefully it's not a problem. If it's not going to be available on one of our platforms, though, I have to imagine that certain other options (librsvg in particular) should be fully cross-platform and should also solve these issues. (I can say that with a fair degree of certainty, since it's what ImageMagick uses for SVG support and ImageMagick (a) has excellent cross-platform support, and (b) had absolutely no trouble rendering the bug sample files properly when I used it to view them.) Really, any general-purpose SVG library should work; my understanding, having now looked into it more, is that QtSVG is not quite a general-purpose SVG library.

Sorry to bombard you with these questions, @N3WWN , just thinking out loud.

ferdnyc commented 6 years ago

I looked into it. They have a build process for Windows and MacOS, so there hopefully shouldn't be any issues. It does mean that rust will need to become part of the OpenShot build environment on those platforms as well. (On Linux that won't be necessary, at least once distros have resvg already built and available as a packaged library.)

They do mention that their Qt backend only "officially" supports Qt >= 5.6. It "should" work with any version, they handwave (prefaced with "Technically," to inspire maximum confidence), but this is another excellent argument for updating the version of Qt used for the AppImage build to something newer than 5.2.1. (IIRC Windows 2.4.1 uses 5.6.something, so that's good enough. I'm blindly assuming MacOS is current enough as well.)

N3WWN commented 6 years ago

I wish I could reply to each message individually, but alas, I have to reply with some context quotes (see also @ferdnyc 's comment of "Github is confusing"). 😄

@ferdnyc :

I'd suggest submitting OpenShot/libopenshot@16b9cd9 as a separate pull request

This should already be a separate pull request under these two PRs: https://github.com/OpenShot/libopenshot/pull/82 and https://github.com/OpenShot/libopenshot/pull/86 (which fixes builds on Mac OSX)

Regarding resvg, yes, it is very new, but I tested out several different methods of rendering SVG files and resvg provided the most features in a relatively small library (do one thing and do it well) that interfaced well with the existing code.

I have updated the build scripts on my dev system to include the resvg library in the AppImage, so it would not be necessary for the individual distro to have resvg available. This would mean that distro-specific builds would lack the full rendering, so perhaps this functionality should be a runtime configuration item (detect the presence of the library and use it, if present, or set the path for it like blender and inkscape in the preferences).

That definitely shouldn't say "FFmpeg" there ( https://github.com/N3WWN/libopenshot/commit/fc4860bd8d1686ff2fabfdfd15164c96c11c6d66#r28778119 , https://github.com/N3WWN/libopenshot/commit/fc4860bd8d1686ff2fabfdfd15164c96c11c6d66#r28778119 )

Oops! You're right! I need to clean up those files!

...ensure that it's only looking at the very end of the string ( https://github.com/N3WWN/libopenshot/commit/fc4860bd8d1686ff2fabfdfd15164c96c11c6d66#r28776960 )

Good catch! I definitely need to fix that!

"Path manipulation" described here : https://github.com/N3WWN/libopenshot/commit/fc4860bd8d1686ff2fabfdfd15164c96c11c6d66#r28777053 and here : https://github.com/N3WWN/libopenshot/commit/fc4860bd8d1686ff2fabfdfd15164c96c11c6d66#r28778078

I will look into this, too!

"printf" described here : https://github.com/N3WWN/libopenshot/commit/fc4860bd8d1686ff2fabfdfd15164c96c11c6d66#r28777146

Yup, obviously debugging code that I had in place that also needs to be cleaned up or removed . It's this kind of thing that is hard to see in your own code and why I didn't submit a PR without getting feedback from you guys 😁

@peanutbutterandcrackers

...just a humble little check-list

Thanks for the link to the list! I had not seen this before, either, and may help me find items to work on next!

I'm afraid I won't be able to test this out as I am working from a friend's computer even right now.

If you're running a Linux machine, I can upload an AppImage for you (or anyone) to test.

@peanutbutterandcrackers and @ferdnyc

I don't have the dev env to build for Mac OS or Windows, but resvg is cross-platform (one of my requirements for the external SVG renderer).

@ferdnyc

Sorry to bombard you with these questions

Bombard away! Most of OpenShot is outside of my wheelhouse, but I use it all the time and any improvements that I can help with help me and, presumably, everyone else who uses it.

I'll take the feedback and update my PR as time permits (not sure what this week looks like yet, so it may or may not be today or tomorrow).

ferdnyc commented 6 years ago

I wish I could reply to each message individually, but alas, I have to reply with some context quotes (see also @ferdnyc 's comment of "Github is confusing").

Yeah... and some of that confusion stems from the odd things they've left out. F'rinstance, I really don't understand why, in the Issues web interface, even when you select some text and use the 'r' shortcut to start a quoted reply, it doesn't attribute the quoted text at all. I won't even bother wishing for real threading, but who figured bare, unattributed blockquotes would be very useful?

It's a very strange, seemingly social-media-influenced conversation structure. (With maybe the odd random feature plucked from email / more traditional online discussion, like the fact that you can quote previous "comments" at all.) Which I suppose may be liked for its familiarity, if it's the only style of online interaction you've ever known, but it's just not as good a fit for bug-tracking and discussion as they seem to imagine. Or maybe I'm just an old fart. Well, no, not maybe.

Yup, obviously debugging code that I had in place that also needs to be cleaned up or removed . It's this kind of thing that is hard to see in your own code and why I didn't submit a PR without getting feedback from you guys

*nod* Though, having tried out the external-repo feedback thing for the first time, now, it's... not ideal. It should be simpler, but Github makes certain things a pain when trying to refer/interact across users' forks. Their concept of request-for-feedback code seems to want it presented as a PR. That gets it in front of the group (directly), enables the full power of Github's review flows (without the "oh, I can't comment here, but I can comment there..." struggle when going against The Github Way™), and... well, hell, it's even nice just to be able to refer to the proposed code by writing " #1384 ", instead of having to puzzle out cross-repo refs/links. :grin:

You can always mark the PR "WIP:" to show that the code isn't ready or intended to be merged, like I did with #1384 until I was confident it was ready to go. (There's even a Github WIP app someone wrote that formalizes the process: It runs a bot that watches for PRs with "WIP" or "do not merge" in the text and sets them to pending status. Mostly that means it disables the Big Green Merge Button, looks like. So, in theory @jonoomph could install that in the repo to manage evolving PRs until they're ready for primetime.)

In the end, tho, I think just getting the code out there, however it's done, is more important than the bureaucracy of how. So thanks for doing that, and looking forward to the next updates on this one!

N3WWN commented 6 years ago

Yeah, I agree with everything you've said, so I must be just as old of a fart as you! 😆

So, for the sake of the flow that we're looking at with OpenShot, would you prefer I make a "WIP" PR for us to continue the discussion?

ferdnyc commented 6 years ago

I think it might be more convenient for us all to be able to discuss the changes that way, and it'd help corral the discussion into a single location, yeah. See @DylanC 's #1506 for an example of the back-and-forth we've been doing on that PR over the past few days.

It sounds like this is getting pretty close, so I don't know that there'll need to be that much more discussion, but when you've got something ready for us to kick around again... I'd say shoot it over as a PR, sure. But that's purely my opinion, speaking only for myself. I don't know if @jonoomph has any particular views on in-development PRs, but I 'd say his opinion should carry the most weight since he's really the intended audience for them all, ultimately.

N3WWN commented 6 years ago

Ah, I see... I can edit the title of the PR after I submit it, so I can prepend "[WIP]" to the PR title and remove it when it is ready.

I'm going to go ahead and submit a PR for this, noting it WIP, for us to continue. Since I'll reference this issue, the bread crumbs should lead us where we need to go.

[If @jonoomph says, "No WIP PRs, please," we'll deal with that at that time 😉 ]

Thanks @ferdnyc !

peanutbutterandcrackers commented 6 years ago

@N3WWN - Yes, please! An AppImage would be great! Also, I am glad that the checklist could be of some help. :)

N3WWN commented 6 years ago

@peanutbutterandcrackers (and anyone else who wants to give it a try), give this one a "Shot":

https://www.dropbox.com/s/5qrswr9yfqf2ipa/OpenShot-v2.4.1-65-g0b47fd8-59-688-x86_64.AppImage?dl=0

I built this one on Apr 25th and have been using it for my videos ever since. It has resvg support built in as well as SHIFT+LEFT and SHIFT+RIGHT to nudge selected clips.

Not sure how long I'll keep it up on Dropbox, but I'll keep it there for at least a week.

ETA: That AppImage is using the externally cached and loaded rendered images a la https://github.com/OpenShot/libopenshot/pull/88/commits/b73ef9b7383602727604dc6f032948a5088c6496 . For the user experience, it's exactly the same as the current state of the PR... the current PR is just more efficient.

peanutbutterandcrackers commented 6 years ago

@N3WWN - Thank you! wget -c-ing it right now (I don't have the best Internet connection :D )

peanutbutterandcrackers commented 6 years ago

@N3WWN - I'm sorry I still haven't tested it out yet. But this morning, I had this thought - why don't we use what inkscape uses to render svg files. And turns out, inkscape uses cairo to render stuff. Do you think OpenShot could also use Cairo? Just a thought. I have to confess I don't know very much about all this...

N3WWN commented 6 years ago

@peanutbutterandcrackers - resvg can use either Qt or Cairo backends. Since OpenShot already uses Qt, that seemed like a natural fit rather than add another external dependency.

peanutbutterandcrackers commented 6 years ago

@N3WWN - Oh, I see... Hmm. I will try to test the AppImage out, soon. :)

peanutbutterandcrackers commented 6 years ago

@N3WWN - So, I did try out the AppImage. Will probably do some further tests but so far I have found only one svg file that it couldn't render. All the others - it rendered really well. You're awesome, as always! However, I couldn't render this one particular svg file. I wonder why that is...

N3WWN commented 6 years ago

@peanutbutterandcrackers - I'm getting the following errors from resvg for All-times-exist-together_nevit_126.svg:

Warning (in svgtypes::style::parser:201): Style attribute '-inkscape-font-specification' is skipped. Warning (in svgtypes::style::parser:201): Style attribute '-inkscape-font-specification' is skipped. Warning (in svgtypes::style::parser:201): Style attribute '-inkscape-font-specification' is skipped. Warning (in svgtypes::style::parser:201): Style attribute '-inkscape-font-specification' is skipped. Warning (in usvg::preproc:113): File doesn't have 'width', 'height' and 'viewBox' attributes. Automatic image size determination is not supported. The Document will be cleared. Warning (in usvg::convert:66): Invalid SVG structure. An empty tree will be produced.

I'm no SVG specification expert, but it looks like the height and width attributes are set to a %, which is fine if there's an existing window/frame/viewport/etc to render the image within, but as a standalone image, that doesn't appear to work without a viewBox.

I did run each of the svg files that comes with OpenShot through resvg the other day and, while I did get some of the same Style attribute '-inkscape-font-specification' is skipped warnings, they all rendered properly. The only exception being Creative_Commons_1.svg, where the banner text (black on white) is not visible:

image

It should look like this:

image

Since Creative_Commons_2.svg renders fine, I'll have to see what the issue is with the first.

N3WWN commented 6 years ago

@peanutbutterandcrackers - I just confirmed that it was the height and width specified as percentages. Here's the same file with width="100%" replaced with width="819" and height="100%" replaced with height="1353" (to mimic the sized of the original): All-times-exist-together_nevit_126_rda.svg

peanutbutterandcrackers commented 6 years ago

Hmm.... I see.

However, the photo manager could render it pretty well. Even on this LXLE machine, mirage, the default photo manager can open it well. And so can InkScape. Also, the Creative_commons_2.svg thing might be a problem. I wonder why resvg can't render it properly.

Perhaps you should file an issue regarding this over at resvg's repo?

ferdnyc commented 6 years ago

@peanutbutterandcrackers - I just confirmed that it was the height and width specified as percentages.

That's supposed to be OK, though, and indeed the original file passes every SVG validator I can find with flying colors.

Because SVG files are scalable, you shouldn't have to specify pixel dimensions for a file, as it can be rendered at any pixel dimensions you want. There really are no "native" pixel dimensions for a fully-vector SVG file. There may be "nominal" dimensions that control the coordinate system used within the file, but those dimensions may also be specified in units other than pixels.

width="100%" is, in a way, actually preferable as a way of specifying an SVG file's dimensions, as it simply means "render to fill available space". That's why, for example, the file shows up with a preview icon in my file manager: The thumbnailer knows that it's supposed to render icons at 128x128 pixels, so it just does, and implicitly width="77" and height="128" for that render. (Not quite the proportions of the original, because like most renderers, including Wikimedia's, the file actually renders with some random blank space to the right and bottom of the text.)

The percentage dimensions also makes that file a good test of rendering capabilities, though, because while it should be OK, it isn't always. OpenShot isn't the only software that has problems with that file. ImageMagick's display command line tool insists on rendering it at 1px × 1px by default, and I can't seem to convince it otherwise with any combination of flags. I consider that a bug in ImageMagick, to be honest.

It's certainly not out of the question to say, "SVG files used in OpenShot must be sized for the video frame", if necessary. But, if it's possible it would be nice NOT to have to do that, especially with video resolutions being so variable recently. I'd love it if the same .svg files we use for HD could be used unmodified on 4K or higher videos, and they'd just render at the native resolution of the output.

I haven't read the code, so apologies if I'm making bad assumptions here. But I'm guessing that, today, if we're expecting pixel dimensions inside the SVG files, that doesn't work? It'd be awesome if it could be made to work, though I'd say it's only worth it if it can be done without too much effort. If there's any way to pass a target output geometry to resvg, when rendering, then would handing it the width and height of the current output profile help with rendering files like that one?

If it isn't easily solvable, then I see no problem with just mandating that SVG files used in OpenShot titles have to contain concrete physical dimensions, as we know that works well and it's a perfectly reasonable restriction considering we're rendering to fixed pixel sizes anyway.

ferdnyc commented 6 years ago

The only exception being Creative_Commons_1.svg, where the banner text (black on white) is not visible:

That's the file. About to open a PR that should fix it. (ETA: #1600)

peanutbutterandcrackers commented 6 years ago

Perhaps the resvg devs should be informed about this? o.O

ferdnyc commented 6 years ago

About which? The Wikimedia SVG or our broken Illustrator-mangled title file?

The Wikimedia file is only a problem if resvg has the same issue as ImageMagick, and fails to render it at set dimensions when those dimensions are specified. I'm not yet clear on whether that's the case, hopefully @N3WWN can shed some light.

(That file, while (as I said) technically valid, is also wonky in other ways. Like, it loads in Inkscape, but if you look at what you're seeing there, it's pretty bizarre. Inkscape sets up page dimensions of 1×1 (either pixels or millimeters, I forget which), then renders the drawing totally outside of that area. So if you use "Zoom to Page" you get this tiny, empty box, and there's a giiiiiiiiiiant drawing way offscreen to the lower ~left~ right. If you "Zoom to Drawing", there's a teeeeeny almost invisible page-area floating to the upper left of the drawing. It's just screwy.)

Our wonky title file, I suppose they might be interested, possibly in adding <switch> to their test suite if nothing else. But at the same time, it's not really within their stated goal featureset to support weirdo extensions like that, so I suspect it will continue to fail that test, and the official word will be, "Not supported, not going to be." Which is perfectly fair. Still, like I said, it may be of interest to them. (ETA: Actually, come to think of it, before bothering them we should check that it isn't already a known-fail part of their test suite.)

peanutbutterandcrackers commented 6 years ago

About the file where the height and width were specified as 100% instead of pixels. But now that you say it and I have tested it on InkScape... I see what you mean. Perhaps it is not necessary to file an issue, then.

I hope resvg will soon support anything and everything the svg standard has specified and will become a one-stop-solution for everything svg.

ferdnyc commented 6 years ago

Mmm, and re: the breaking version of our Creative_Commons_1.svg, it turns out that, while <switch> is standard SVG, as the SVG spec itself notes:

Usually, a ‘foreignObject’ will be used in conjunction with the ‘switch’ element and the ‘requiredExtensions’ attribute to provide proper checking for user agent support and provide an alternate rendering in case user agent support is not available.

It was the foreignObject within the <switch> that utilized an Illustrator extension. And, indeed, resvg is documented as not supporting or properly handling foreignObject at this time.

N3WWN commented 6 years ago

All of the existing svg files that come with OpenShot have fixed widths and heights specified, so I don't think it's a show stopper that resvg does not currently support images with dynamic sizes.

I'd love it if the same .svg files we use for HD could be used unmodified on 4K or higher videos, and they'd just render at the native resolution of the output.

Maybe we could do this by adding a tad of parsing to the svg data before it is sent to resvg for rendering? If a % height or width is specified, replace that percentage with a fixed value.

I have looked at obtaining project/timeline info within the readers (QtImageReader in particular), and as far as I can tell, it's not available. But if it is, or can be made so, it will allow us to fix this and other issues, like https://github.com/OpenShot/openshot-qt/issues/1478#issuecomment-388176705, as well.

mcbluefire commented 6 years ago

This problem continues in 2.4.3 and 2.4.3-dev1 that I just downloaded tonight. I even created a multiple line title in the latest version and it instantly converted to a title and the multi-line subtitle show as one continuous line.

assets.zip

ferdnyc commented 6 years ago

This problem continues in 2.4.3 and 2.4.3-dev1 that I just downloaded tonight. I even created a multiple line title in the latest version and it instantly converted to a title and the multi-line subtitle show as one continuous line.

That's not a rendering thing, that's a structure thing. If you noticed, loading the file into the Title Editor showed the file as containing three lines (with "Title", "Line1", and "Line2" filled in). But in Inkscape, you only had two text boxes: lines 2 and 3 ("Line1" and "Line2") were in the same two-line box, which confused OpenShot. Changing the rendering engine wouldn't change that.

This version of the file splits that box into two separate boxes, so there's one per each of the three Line fields in the Title Editor. It renders fine in OpenShot. (I just roughly eyeballed the placement of the third line ("Line2"), so that may have to be adjusted. It's definitely not in exactly the same place as it was in the original version.)

TitleFileName-2.svg.zip

Also, just as an aside, those drop shadows on the text aren't going to be rendered. OpenShot doesn't support the "filter" attribute, so they're just ignored. (ReSVG doesn't support filter either, so that's another thing that's not going to change.) You can remove them with the "Remove Filters" option at the bottom of the Filters menu in Inkscape to make the file simpler, but even if you leave them there the rendered titles aren't going to have drop shadows.

mcbluefire commented 6 years ago

@fernyc, Thanks for the reply. I appreciate all the details in your response. What is interesting are the following: 1) Openshot doesn't support the filter attribute so the drop shadows won't be rendered. This is the "Gold 2" title template supplied by Openshot 2.4.3 with one line added via "advanced editor" option in OpenShot.

2) Openshot's title editor is what is showing there are more than one line - so why doesn't OpenShot itself render with more than one line? The OpenShot title editor and the OpenShot renderer should be on the same page. Nautilus shows the svg file correctly, image viewer shows it correctly, firefox shows it correctly, OpenShot 1.4.3 shows it correctly and ...; I'd be hard pressed to find anything that shows it incorrectly other than OpenShot 2.4.3. That has to be an OpenShot rendering issue: what else could it be? Newlines in a text box is not outside of the standard - although technically it is a completely new tspan complete with x and y position that are being ignored by the renderer in OpenShot.

3) I appreciate that I can just add a new text field for every line and will likely have to do that or make a png of every title and replace them in the timelines until Openshot can render an svg per standards. Now I need to go back and edit 140 titles I just created two weeks ago using Openshot 1.4.3 and Inkscape/"Use Advanced Editor" because Openshot 2.4.3 cannot properly render an SVG file that was created using tools supplied with Openshot 2.4.3 and previous.

4) There is an additional issue in the rendering. Like I said, I have 140 titles. All 140 were created using "Gold 2" title template in 1.4.3. They all draw perfectly in everything but OpenShot 2.4.3. Once I view them in OpenShot 2.4.3 they go from title/multi-line subtitles centered to tittle/single-line subtitles justified left from the center of the screen. Even if there was only one line in the sub-title it still goes from centered to left-justified from the center. Granted they are using the text-anchor:start and text-aligment:start tags as created by inkscape, but they render fine anywhere other than OpenShot 2.4.3. openshot - svg 2 4 3 screenshot from 2018-10-03 11-29-54

I moved to Openshot 2.4.3 because I was running into several cut issues with 1.4.3. I'd cut a scene precisely to the frame between scense (using left-right arrows and 'c') and then rarely when I reviewed the clip it would go for another a second or two into the next scene. I'd then have to cut it again and move the sliver to the next scene. Kind of rough working with tiny clips of a second or less. 2.4.3 makes it easier to move slivers like that around and even better just resize the clip to include/exclude as necessary. I do not have the luxury of moving back to 1.4.3 as once the autosave happened to the project file OpenShot 1.4.3 no longer can use the project file.

Appreciate your earlier response and hope my additional feedback/thoughts help.

ferdnyc commented 6 years ago
  1. Openshot doesn't support the filter attribute so the drop shadows won't be rendered. This is the "Gold 2" title template supplied by Openshot 2.4.3 with one line added via "advanced editor" option in OpenShot.

OpenShot is supplying titles with drop shadows / filters? Hm. So it is. Specifically:

$ cd src/titles; grep -c filter *.svg|egrep -v ':0$'|cut -d: -f1
Bar_3.svg
Gold_1.svg
Gold_2.svg
Gold_Bottom.svg
Gold_Top.svg
Post_it.svg
Sunset.svg

Note to OpenShot devs: Probably shouldn't be doing that, it's just misleading.

  1. Openshot's title editor is what is showing there are more than one line - so why doesn't OpenShot itself render with more than one line? The OpenShot title editor and the OpenShot renderer should be on the same page.

Well... you're right. There are conflicts in the way things interact. And I may be wrong that changing the rendering engine wouldn't change this, because (as you saw) OpenShot parses the contents of the SVG based on TSPAN tag, and finds three lines. But QtSVG's very simple renderer only supports SVG Tiny, and TSPANs with positional properties are outside the SVG Tiny spec, so QtSVG ignores those properties and renders the second TSPAN right after the first, because they occupy the same text box. (As that bug notes, Qt are open to accepting patches that add support for positional TSPANs to QtSVG, but currently nobody's working on those patches.)

As things currently stand, addressing this in OpenShot would probably take the form of parsing based on TEXT tags, rather than TSPANs, in detecting what constitutes "a line". That would match the output of the renderer. But such a change wouldn't solve your problem, it would merely get OpenShot, as you say, "on the same page" as the QtSVG renderer.

That has to be an OpenShot rendering issue: what else could it be?

A QtSVG / SVG Tiny limitation.

Now I need to go back and edit 140 titles

As far as actually solving that problem, if the titles all take the same basic form (one or more of their TEXT tags encloses multiple TSPANs positioned to represent multiple lines), if you're able to zip them all up and attach them to this issue I can fix up the structure en masse with a little Perl.

Once I view them in OpenShot 2.4.3 they go from title/multi-line subtitles centered to tittle/single-line subtitles justified left from the center of the screen. Even if there was only one line in the sub-title it still goes from centered to left-justified from the center. Granted they are using the text-anchor:start and text-aligment:start tags as created by inkscape, but they render fine anywhere other than OpenShot 2.4.3.

I'd have to see the structure, but since the start value corresponds to left-alignment (or right-alignment in RTL text), there must be a text-align:center or text-anchor:middle somewhere. In the title file you attached, all of the TEXT tags used text-anchor:middle and text-align:center. As did all of the TSPANs, though as a positional attribute, I believe that also isn't supported under SVG Tiny. Still, didn't matter as the TEXT tags were set correctly.

If you're seeing centered text in other applications, my guess is that the TEXT tags are set text-align:start / text-anchor:start, and the TSPANs individually have text-align:center / text-anchor:middle properties, a configuration which would cause left-alignment in QtSVG. Regardless, that's something that I should also be able to fix en masse.

ferdnyc commented 6 years ago

As a side note, I'd personally love it if Inkscape supported SVG Tiny as an output format, or better yet an input/editing mode, and restricted the featureset accordingly to avoid creating files which use features not supported in SVG Tiny. There was some discussion of SVG Tiny compliance and/or output around 10 years ago, but nothing seems to have come of that.

mcbluefire commented 6 years ago

@ferdnyc Appreciate all your responses. You are definitely correct the TEXT tags have settings for center and middle. :+1: My guess is from 1.4.3 to 2.4.3 OpenShot devs made a major decision to shift to another less feature-rich svg renderer. Wonder what happened to the old svg engine. :-1:

I'm happy to do some perl, sed or other scripted replacements en masse on the files myself. You appear to know the svg markup pretty well. Would I just be needing to replace the tspan tags with text tags so QtSVG can work with them? Appreciate any insight. Also, if you have somewhere we can take this offline, I'm good with that.

ferdnyc commented 6 years ago

You appear to know the svg markup pretty well. Would I just be needing to replace the tspan tags with text tags so QtSVG can work with them?

Not replace, but wrap. Each <text> tag does still need to contain a <tspan>. (If for no other reason than OpenShot's Title Editor looks for them, and not having them would break it.) QtSVG just can't position text based on <TSPAN>s. Positioning is only supported on <TEXT> tags under SVG Tiny, so each <TSPAN> that repositions its contained text needs to be wrapped in its own <TEXT> tag with that same positioning.

So you'd want to take the chunks that contain:

<text>
  <tspan>Line 1</tspan>
  <tspan>Line 2</tspan>
<text>

And turn them into:

<text>
  <tspan>Line 1<tspan>
</text>
<text>
  <tspan>Line 2</tspan>
</text>

The only tricky bit is the position handling. Where you'll currently have (from the sample file you provided) e.g.:

<text style="textstuff" x="959.01288" y="544.8371">
  <tspan style="tspanstuff" x="959.01288" y="544.8371">Line 1</tspan>
  <tspan style="tspanstuff" x="959.01288" y="647.77881">Line 2</tspan>
</text>

You'll want to update that to:

<text style="textstuff" x="959.01288" y="544.8371">
  <tspan style="tspanstuff" x="959.01288" y="544.8371">Line 1</tspan>
</text>
<text style="textstuff" x="959.01288" y="647.77881">
  <tspan style="tspanstuff" x="959.01288" y="647.77881">Line 2</tspan>
</text>

(IOW, you'd want to steal the non-positional properties from the <TEXT> tag being duplicated, but the positional properties from the <TSPAN> being enclosed.)

text-align:start/text-anchor:start vs. text-align:center/text-anchor:middle, same basic deal. You'd want to either remove the text-align:start or text-anchor:start style attributes from any tag (at any level) that encloses something that should be centered but isn't, or you'd want to replace them with text-align:center and text-anchor:middle, since those attributes aren't being respected from a different level. The latter is probably the more forceful approach, and therefore more likely to succeed in convincing all parsers, including QtSVG.

My guess is from 1.4.3 to 2.4.3 OpenShot devs made a major decision to shift to another less feature-rich svg renderer. Wonder what happened to the old svg engine.

Yeah, I don't know the 1.4.x code at all, so I'm not sure what exactly handled rendering in those versions. OpenShot 2.0 was a complete rewrite — there isn't a single line of code that remains from the 1.x series, except for a few legacy class files preserved solely to handle importing and conversion of old 1.4.x project files.

In particular, 1.x was written entirely in Python, it used GTK for the interface and a Python library called MLT for most of the video lifting. Presumably either MLT or GTKPixbuf would've handled SVG rendering.

OpenShot 2.x uses Qt for the GUI and a lot of the media file management, and hands the rendering (and playback) of A/V content off to its own C library built on top of FFMpeg. Whatever renderer was used in the 1.x versions wouldn't be directly available to libopenshot. Whether it was MLT or GTK, neither is used in OpenShot 2.0, and neither provides a way to access just an SVG rendering functionality independent of the rest of the framework. Pulling in an entire codebase like that just for SVG support isn't practical. Especially since it would have to be part of libopenshot, not the OpenShot frontend.

QtSVG is unfortunately very limited by design (moreso than the devs probably realized initially), but it's also the only option Qt provides, and thus the only option that didn't require code to integrate it or add external dependencies to libopenshot. (The alternate renderer proposed here, ReSVG, is extremely capable. But as a rust library, it also adds numerous dependencies, since rust is a highly modular language. And as I only just happened to notice, it's also not available for 32-bit Windows, which adds a further wrinkle.)

mcbluefire commented 6 years ago

Many thanks, I got them to work alright by commenting out the text tags, replacing tspan with text and fixing the text alignments.

for file in *.svg do sed -i 's/text-align:start/text-align:center/' $file sed -i 's/text-anchor:start/text-anchor:middle/' $file

sed -r -i 's/(<text [^>]*>)/\1 -->/' $file sed -i 's/<text /<!-- <text /' $file sed -i 's/<\/text>//' $file

sed -i 's///' $file sed -i 's/<\/tspan>/<\/text>/' $file done

If anyone decides to use this please note the following: Disclaimer - didn't add any logic to prevent running on a file more than once - doing so will definitely break it. Backup your svg files before running this so you can just copy from backup and run again if anything goes wrong.

If I run into any issues I'll update to include tspan tags within each text tag. Thus far my most complicated multi-line file translated perfectly.

Thanks again for all your help - greatly appreciated.

ferdnyc commented 6 years ago

Many thanks, I got them to work alright by commenting out the text tags, replacing tspan with text and fixing the text alignments.

Excellent, glad it worked out. That should be fine, provided you don't need the ability to edit the titles in OpenShot's Title Editor interface. (If you were to load them into the Title Editor, you'd find that the TEXT tags without TSPAN children aren't detected. But I'm fairly sure that should be the only issue, and as I said it's only an issue if you actually need them to be editable.)

If I run into any issues I'll update to include tspan tags within each text tag. Thus far my most complicated multi-line file translated perfectly.

*nod* That's all you'd need to do, at least for editability. The Title Editor expects a single <tspan> for every <text>, and it requires a style="" attribute on that <tspan> (because it parses and modifies the inline CSS within). But wrapping the content inside the <text>...</text> block with a simple <tspan style="">...</tspan> should suffice to re-enable interactive editing.