Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Embed images in SVG output #189

Merged
merged 1 commit into from
Jun 7, 2023
Merged

Embed images in SVG output #189

merged 1 commit into from
Jun 7, 2023

Conversation

formatc1702
Copy link
Collaborator

@formatc1702 formatc1702 commented Oct 24, 2020

[Update 2020-11-16]

This PR has evolved from simply fixing how relative image paths are resolved, to embedding any images in the SVG file by default, eliminating the original issue.


Previously, paths to images were resolved relatively to the specified output file instead of the input file.

This produces errors when calling WireViz with the -o flag, creating output files in a different location from the input.

This PR [partially, see comment below] fixes the problem.

Additionally, it eliminates the need for passing a gv_dir variable to the Image class, resolving paths beforehand and passing the absolute path to the Image class.

@formatc1702 formatc1702 marked this pull request as draft October 24, 2020 19:59
@formatc1702
Copy link
Collaborator Author

It seems GraphViz only links to the image in the SVG output. This might be problematic if the output is stored in a different directory from the input, or moved afterwards. Further investigation is required.

@formatc1702 formatc1702 reopened this Oct 24, 2020
@formatc1702
Copy link
Collaborator Author

formatc1702 commented Oct 24, 2020

@kvid I'd be interesting in your input on this topic. The solution would be complete if it wasn't for the fact that the SVG, and by extension, the HTML output, depend on having a working link to the original image file.

The .gv file is also affected, but this should be less of a problem for the average user, since this is just an intermediate file.

It seems that GraphViz does not offer a way to embed images?

@formatc1702
Copy link
Collaborator Author

formatc1702 commented Nov 1, 2020

Here are some new discoveries that might fix the above mentioned problem with embedded images.

  • HTML <IMG> tags allow embedding base64-encoded images within the SRC attribute, avoiding the need to link to external files: example 1; example 2 (encoded by me using base64-image.de)
  • GraphViz does not seem to support this. A quick test (using example 1 above) reveals that
    attribs['image']['src'] = 'data:image/png;base64, iVBORw0KGgoAAAANSUhEUgAAAAUAAAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO9TXL0Y4OHwAAAABJRU5ErkJggg=='
    
    produces an error:
    graphviz.backend.CalledProcessError: Command '['dot', '-Tpng', '-O', 'ex08']' returned non-zero exit status 1. [stderr: b'Warning: No such file or directory while opening data:image/png;base64, iVBORw0KGgoAAAANSUhEUgAAAAUAAAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO9TXL0Y4OHwAAAABJRU5ErkJggg==\nError: No or improper image file="data:image/png;base64, iVBORw0KGgoAAAANSUhEUgAAAAUAAAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO9TXL0Y4OHwAAAABJRU5ErkJggg=="\nin label of node Key\nWarning: No such file or directory while opening data:image/png;base64, iVBORw0KGgoAAAANSUhEUgAAAAUAAAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO9TXL0Y4OHwAAAABJRU5ErkJggg==\nError: No or improper image file="data:image/png;base64, iVBORw0KGgoAAAANSUhEUgAAAAUAAAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO9TXL0Y4OHwAAAABJRU5ErkJggg=="\nin label of node W1\n']
    
  • Inserting the data manually into an SVG file's <image xlink:href=""> attribute works! 🎉
    Here is ex08.svg with both xlink:href attributes containing the original images in base64 encoding. Note that the image renders correctly, even though the gist does not include any bitmap files.
  • This would fix the problem for SVG, and by extension, HTML output. GraphViz output can't be fixed, but I see this as a minor problem.
  • More about data URIs

@kvid do you think it's a reasonable approach to convert and inject the images into the output SVG file in this way? If so, this PR should only fix the relative path issue, and I'll begin another one to work on the encoding.

@formatc1702 formatc1702 marked this pull request as ready for review November 1, 2020 15:27
@formatc1702 formatc1702 added this to the v0.3 milestone Nov 1, 2020
@kvid
Copy link
Collaborator

kvid commented Nov 6, 2020

I'm a bit busy these days, and are not able to contribute as often as I want.

Here are some new discoveries that might fix the above mentioned problem with embedded images.

  • HTML <IMG> tags allow embedding base64-encoded images within the SRC attribute, avoiding the need to link to external files: example 1; example 2 (encoded by me using base64-image.de)

That looks useful, but I don't think it's much in use. I've never seen it in an HTML page.

  • GraphViz does not seem to support this. A quick test (using example 1 above) reveals that

    attribs['image']['src'] = 'data:image/png;base64, iVBORw0KGgoAAAANSUhEUgAAAAUAAAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO9TXL0Y4OHwAAAABJRU5ErkJggg=='
    

    produces an error:

    graphviz.backend.CalledProcessError: Command '['dot', '-Tpng', '-O', 'ex08']' returned non-zero exit status 1. [stderr: b'Warning: No such file or directory while opening data:image/png;base64, iVBORw0KGgoAAAANSUhEUgAAAAUAAAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO9TXL0Y4OHwAAAABJRU5ErkJggg==\nError: No or improper image file="data:image/png;base64, iVBORw0KGgoAAAANSUhEUgAAAAUAAAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO9TXL0Y4OHwAAAABJRU5ErkJggg=="\nin label of node Key\nWarning: No such file or directory while opening data:image/png;base64, iVBORw0KGgoAAAANSUhEUgAAAAUAAAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO9TXL0Y4OHwAAAABJRU5ErkJggg==\nError: No or improper image file="data:image/png;base64, iVBORw0KGgoAAAANSUhEUgAAAAUAAAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO9TXL0Y4OHwAAAABJRU5ErkJggg=="\nin label of node W1\n']
    
  • Inserting the data manually into an SVG file's <image xlink:href=""> attribute works! 🎉
    Here is ex08.svg with both xlink:href attributes containing the original images in base64 encoding. Note that the image renders correctly, even though the gist does not include any bitmap files.

  • This would fix the problem for SVG, and by extension, HTML output. GraphViz output can't be fixed, but I see this as a minor problem.

  • More about data URIs

@kvid do you think it's a reasonable approach to convert and inject the images into the output SVG file in this way? If so, this PR should only fix the relative path issue, and I'll begin another one to work on the encoding.

@formatc1702
Copy link
Collaborator Author

I'm a bit busy these days, and are not able to contribute as often as I want.

Don't worry about it, I know the situation.

You don't seem to have an e-mail address associated with your GitHub account (besides [email protected], which I assume won't work)... is there a way for me to send you a direct message? Please let me know via [email protected].. thanks!

@kvid
Copy link
Collaborator

kvid commented Nov 14, 2020

The different approaches each have their advantages and disadvantages:

  • When specifying an output location different from the input, all image files need to have absolute paths or optionally a modified relative path (if possible) or optionally a copy of the image files can be made at the output location. Converting to absolute paths will not work if the output location is not the final destination. I guess the user should have all 3 options.
  • Some users might find it useful to allow a data URI as image source input, but it must be converted to a file for Graphviz to find it, and as in the previous point, absolute path or relative path must be selected.
  • Some users might find it useful to generate a data URI in any image source of the SVG output, but I guess it should be optional per image independently of the options above (and maybe a harness option to be used when not specified for each image). The clean way is to make this a new output (e.g. .data.svg) to avoid renaming or caching the Graphviz generated .svg before overwriting the same output name with the modified version containing data URIs.

@formatc1702
Copy link
Collaborator Author

formatc1702 commented Nov 14, 2020

Ideally, the output files should be self-contained, independent of any paths to other files. Anything else just causes more trouble, and the added effort of specifying behaviors (as per the first bullet point of your post) is not really worth it IMHO.

[Edit]
The closest analogy I can think of is including schematic symbols and footprints in an EAGLE project.
Once a library component has been included in a project, the project files can be shared without having to also share the libraries, since everything has been embedded within the schematic/PCB file itself. I find this immensely useful and would therefore like to replicate this behavior.

Therefore, my final goal is to:

  • allow user to specify paths to images inside the YAML, relative (to the YAML file) or absolute
  • resolve relative image paths to absolute ones when parsing the YAML (this PR)
  • generate BOM, PNG and SVG output files
  • modify SVG output, embedding the images in the <image xlink:href=""> tags as described above using base64 encoding.
    • I don't really see the need for keeping the SVG with the reference to an external file, except for debugging purposes.
  • generate HTML output, containing the modified SVG

As you can see, I'm trying to follow the KISS principle instead of providing too much configurability... I'd rather implement this straightforward (implementation-wise), but hopefully foolproof (from the user's perspective) method, and eventuall add more output options later.

@formatc1702
Copy link
Collaborator Author

formatc1702 commented Nov 15, 2020

Embedding images in the output SVG works 🎉

It has been tested with PNG, JPEG and animated GIFs.
(To see the working animation, click the Raw button in the Gist, download the file, open in browser.)

src/wireviz/svgembed.py Outdated Show resolved Hide resolved
src/wireviz/svgembed.py Outdated Show resolved Hide resolved
src/wireviz/svgembed.py Outdated Show resolved Hide resolved
@formatc1702
Copy link
Collaborator Author

formatc1702 commented Nov 16, 2020

@kvid, this PR is ready for review.
Feel free to mark my comments as resolved if you agree, and let me know if you don't.
I am open to discussing your ideas regarding finer user control over how to handle images (embed or not, copy to output directory, ...) as a separate PR, but I would first like to have embedded images as default since it leaves no open questions (see my comment above).
Thanks!

@formatc1702 formatc1702 changed the title [bugfix] Resolve image paths correctly [feature/bugfix] Embed images in SVG output Nov 16, 2020
Copy link
Collaborator

@kvid kvid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry this review took so long time. I only had time for a minor section each day, and some of the days, I had to give up due to extremely slow response from github servers.

@kvid, this PR is ready for review.
Feel free to mark my comments as resolved if you agree, and let me know if you don't.

I'm not allowed to mark your code comments as resolved, but I've tagged with 👍 where I agree.

I am open to discussing your ideas regarding finer user control over how to handle images (embed or not, copy to output directory, ...) as a separate PR, but I would first like to have embedded images as default since it leaves no open questions (see my comment above).
Thanks!

I agree those other ideas can be handled later. Probably will most users will be happy with this PR and might not give my other ideas a high priority.

src/wireviz/wireviz.py Outdated Show resolved Hide resolved
src/wireviz/svgembed.py Outdated Show resolved Hide resolved
src/wireviz/svgembed.py Outdated Show resolved Hide resolved
src/wireviz/svgembed.py Outdated Show resolved Hide resolved
src/wireviz/svgembed.py Outdated Show resolved Hide resolved
src/wireviz/Harness.py Outdated Show resolved Hide resolved
src/wireviz/svgembed.py Outdated Show resolved Hide resolved
src/wireviz/wireviz.py Outdated Show resolved Hide resolved
@@ -209,7 +210,7 @@ def parse_file(yaml_file: str, file_out: (str, Path) = None) -> None:
file_out = fn
file_out = os.path.abspath(file_out)

parse(yaml_input, file_out=file_out)
parse(yaml_input, file_in=Path(yaml_file).resolve(), file_out=file_out)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest moving Path( ).resolve() inside the parse() function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See 6a42a30 for the new implementation and let me know what you think!

@@ -251,7 +252,7 @@ def main():
file_out = args.output_file
file_out = os.path.abspath(file_out)

parse(yaml_input, file_out=file_out)
parse(yaml_input, file_in=Path(args.input_file).resolve(), file_out=file_out)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest moving Path( ).resolve() inside the parse() function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See 6a42a30 for the new implementation and let me know what you think!

Copy link
Collaborator

@kvid kvid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have included a few more suggestions, including a fix for a bug that might lead to bad replacements if one URL is a substring of another, and the former is replaced before the latter. The error is that only the substring of the latter will be replaced. E.g. if "connector.png" and "shielded_connector.png" are two of the URLs in the same SVG, then the "connector.png" part of the latter URL might be replaced by the embedded contents of the former URL.

src/wireviz/svgembed.py Outdated Show resolved Hide resolved
src/wireviz/svgembed.py Outdated Show resolved Hide resolved
src/wireviz/svgembed.py Outdated Show resolved Hide resolved
@formatc1702
Copy link
Collaborator Author

formatc1702 commented Dec 6, 2020

Thanks for your suggestions. Don't worry about not responding earlier.. there is no obligation here.
I've included most of your suggestions and commented on the ones that need discussion/clarification.

Copy link
Collaborator

@kvid kvid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your suggestions. Don't worry about not responding earlier.. there is no obligation here.
I've included most of your suggestions and commented on the ones that need discussion/clarification.

Thank you for accepting most of my suggestions. I've tried answering the issues with questions, and added some more suggestions as well.

src/wireviz/Harness.py Outdated Show resolved Hide resolved
src/wireviz/DataClasses.py Outdated Show resolved Hide resolved
src/wireviz/Harness.py Outdated Show resolved Hide resolved
src/wireviz/svgembed.py Outdated Show resolved Hide resolved
src/wireviz/svgembed.py Outdated Show resolved Hide resolved
src/wireviz/svgembed.py Outdated Show resolved Hide resolved
src/wireviz/svgembed.py Outdated Show resolved Hide resolved
src/wireviz/svgembed.py Outdated Show resolved Hide resolved
src/wireviz/wireviz.py Outdated Show resolved Hide resolved
src/wireviz/wireviz.py Outdated Show resolved Hide resolved
src/wireviz/wireviz.py Outdated Show resolved Hide resolved
@formatc1702
Copy link
Collaborator Author

formatc1702 commented Oct 2, 2021

A major drawback with resolving all image paths to the absolute path, is that the output file contents will depend on the absolute path that normally differ between developers, and thereby lead to false diffs when doing regression testing.

The way I see it, this would only affect .gv files.

I agree.

I am working on a refactoring of the WireViz CLI in #244 which will finally close #60 and will stop outputting .gv files by default.
Since you agree that .gv is rarely needed outside development, would you be OK if I implement your other suggestions, ignore the fact that absolute paths will be visible if .gv output is enabled, and we get this PR ready?
I am sure we can then create a follow-up PR to elegantly deal with the case where .gv is generated.

I am using smart_file_resolve() to determine relative image paths.
The current strategy does not consider whether the relative path is defined in the main YAML, or the prepended YAML. In both cases, it searches relative to the main YAML's directory first, and tries relative to the prepended YAML only if unsuccessful.
I realize it is not the absolute cleanest solution, but it is simple, deterministic, and should work in most cases... and might even have some advantages.

@formatc1702 formatc1702 requested a review from kvid October 2, 2021 15:57
@formatc1702 formatc1702 modified the milestones: v0.3, v0.4 and later Oct 7, 2021
formatc1702 added a commit that referenced this pull request Oct 7, 2021
formatc1702 added a commit that referenced this pull request Oct 8, 2021
@formatc1702 formatc1702 changed the base branch from dev to feature/improved-parse October 16, 2021 16:56
@formatc1702 formatc1702 changed the title [feature/bugfix] Embed images in SVG output Embed images in SVG output Oct 16, 2021
@formatc1702
Copy link
Collaborator Author

Rebased on top of latest to profit from smart_file_resolve().
Old branch with full commit history is backed up as backup/bugfix/image-src_before-latest-rebase

@formatc1702 formatc1702 mentioned this pull request Aug 5, 2022
5 tasks
Base automatically changed from feature/improved-parse to feature/cli August 5, 2022 16:06
Base automatically changed from feature/cli to feature/technical-drw August 5, 2022 16:33
Base automatically changed from feature/technical-drw to dev June 7, 2023 18:01
@formatc1702 formatc1702 merged commit 6f9bb67 into dev Jun 7, 2023
@formatc1702 formatc1702 deleted the bugfix/image-src branch June 7, 2023 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants