resolves #507 add png support for Mermaid

[derlin]

Add the /png endpoint support for mermaid diagrams.
The resulting png has transparent background, and has no margin (will really only contain the actual rendered diagram).

See #507

[ggrossetie]

I believe it can be simplified to:

Suggested change

	const outputType = req.url.match(/\/((?:png)|(?:svg))/)?.[1];
	const outputType = req.url.match(/\/(png|svg)/)?.[1];

If needed we could check that /svg or /png is the last segment using $ at the end of the regular expression.

[derlin]

well, just adding $ at the end would still leave /svg/png to pass (and generate a png), so to me either we do complete validation (total match, ^ and $) or none. The alternative would the be:

const outputType = req.url.match(/^\/(png|svg)$/)?.[1];

Should I make the change ?

[ggrossetie]

well, just adding $ at the end would still leave /svg/png to pass (and generate a png), so to me either we do complete validation (total match, ^ and $) or none.

That's a good point.
Yes, I think we should use a strict/complete validation, /svg/png is unclear and we should definitely return an error (not try to proceed).

[ggrossetie]

I left a few nitpicks but overall great work 👌🏻

I would like to add a test on this feature. You can take a look at https://github.com/yuzutech/kroki/blob/main/mermaid/tests/test.js for inspiration.

Comparing PNG images can be tricky but we can use https://github.com/mapbox/pixelmatch to do pixel-level image comparison.

Thanks!

[derlin]

@Mogztter thank you for the review !

I applied the proposed changes, fixed the linter issues and added the missing return statements after micro.send (would make the server crash on missing body otherwise).

I also included two tests about PNG generation using pixelmatch as you proposed. I just had a doubt on where to put the reference images. They are currently under tests, but maybe a subfolder (e.g. tests/resources would be cleaner ?

[ggrossetie]

I applied the proposed changes, fixed the linter issues and added the missing return statements after micro.send (would make the server crash on missing body otherwise).

👍🏻

I also included two tests about PNG generation using pixelmatch as you proposed. I just had a doubt on where to put the reference images. They are currently under tests, but maybe a subfolder (e.g. tests/resources would be cleaner ?

That's a good idea.
Usually, I'm using the word "fixtures" but I don't have a strong opinion on the naming.

[ggrossetie]

Tests are currently failing because the width and height of the PNG images are slightly different. My guess is that rasterization is slightly different (most likely on fonts) and you can get a few pixels difference.

  1) #convert
       should return a PNG equal to: graph.png:

      AssertionError: expected 178 to equal 175
      + expected - actual

      -178
      +175
      
      at Context.<anonymous> (tests/test.js:66:26)
      at processTicksAndRejections (internal/process/task_queues.js:95:5)

  2) #convert
       should return a PNG equal to: seq.png:

      AssertionError: expected 504 to equal 500
      + expected - actual

      -504
      +500
      
      at Context.<anonymous> (tests/test.js:66:26)
      at processTicksAndRejections (internal/process/task_queues.js:95:5)

So we should also use a threshold on the width and height. Something like, more or less 5% of the expected width/height:

// threshold, more or less 5% of the expected widht/height
expect(width).to.be.within(expectedImage.width / 1.05, expectedImage.width * 1.05);
expect(height).to.be.within(expectedImage.height / 1.05, expectedImage.height * 1.05);

[derlin]

Tests are currently failing because the width and height of the PNG images are slightly different. My guess is that rasterization is slightly different (most likely on fonts) and you can get a few pixels difference.

Wow, it passed locally and I didn't think of this. Just to ensure I understood, you mean that depending on the os, puppeteer will generate slightly different images, due mostly to the font rendering?

If this is the case, and if the sizes differ, I fear pixelmatch will fail as well... And I am not sure how to proceed with testing then (appart checking that the png is valid and at least of a given size, as mermaid.ink does)

[ggrossetie]

Wow, it passed locally and I didn't think of this. Just to ensure I understood, you mean that depending on the os, puppeteer will generate slightly different images, due mostly to the font rendering?

Yes, that's correct.
Having said that, we are running the tests suite inside Docker so the runtime environment should be the same. Are you running the tests suite outside of Docker? Are you using macOS?
I will checkout your branch and give it a try on my machine.

If this is the case, and if the sizes differ, I fear pixelmatch will fail as well... And I am not sure how to proceed with testing then (appart checking that the png is valid and at least of a given size, as mermaid.ink does)

pixelmatch is using a threshold so we should be fine as both images are mostly the same (or at least should be):

const numOfDifferentPixels = pixelmatch(resultImage.data, expectedImage.data, diffImage.data, width, height, {threshold: 0.1})

My only concern is that pixelmatch mandates that image dimensions must be equal.
But first thing first I will try to reproduce this issue on my machine.

[derlin]

Are you using macOS?

Yes I am. I thought also of simply making the tests run on docker. However, to me having a test that fails locally but succeeds on docker build is more problematic than not. I imagine the next developer cloning the repo, and spending 3h understanding why the tests suddenly fail, because he didn't think of trying to run it inside docker.

But anyways, as you said, first things first: let's see if pixelmatch can handle different sizes (will work on it after work). Will keep you posted!

[derlin]

@Mogztter Ok, so I investigated a bit.

The SVGs in docker have a more padding below the text, making all shapes grow a bit in height. This explains the extra pixels and is not due to any change in CSS: comparing the two SVGs on the same browser, I can see the computed size of the shapes, hard-coded, are different, so it is during mermaid rendering. I thought it could come from the Chromium version, which is different between docker and my Mac. I tried using the latest alpine image, 3.14.2, to no avail (I don't think the patch difference could explain this gap):

           mac: HeadlessChrome/93.0.4577.0
        docker: HeadlessChrome/86.0.4240.111
docker updated: HeadlessChrome/93.0.4577.82

Also, pixelmatch requires the two images to be exactly the same size.

what I propose: since the png feature is only about taking a screenshot from the svg, we could argue that if the svg is rendered correctly, the png should as well. In PNG tests, we should thus only ensure that 1) the worker doesn't throw an error, and 2) the returned PNG is valid:

  pngTests.forEach((testCase) => {
    it(`should return a valid PNG from content: ${testCase.content}`, async function () {
      const browser = await puppeteer.launch({args: ['--no-sandbox', '--disable-setuid-sandbox']})
      try {
        const worker = new Worker(browser)
        const result = await worker.convert(new Task(testCase.content, true));
        
        const image = PNG.sync.read(result); // this will fail on invalid image
        const {width, height} = image;
        
        // ensure we have some content in the PNG
        expect(image.data.length).to.be.greaterThan(100);
      } finally {
        await browser.close()
      }
    })
  })

Also, I was looking at the SVG generation, and since we are taking the content of #container, which is always present, but don't check if it has an svg child, it means an invalid graph (with syntax error) returns a 200 OK for /svg (but fails for /png). We could change it by adding those lines before the if(isPng) in the worker, so we actually get a 400 BAD REQUEST on invalid syntax:

      const svg = await page.$('#container > svg')
      if(svg == null) {
        throw Error("Invalid syntax in graph (no svg found)")
      }

Finally, I am wondering if I should update the alpine image since we are at it, or do another PR for that...

What do you think of all this ?

[ggrossetie]

Wow, thanks for taking the time to look at it closely, great work!

what I propose: since the png feature is only about taking a screenshot from the svg, we could argue that if the svg is rendered correctly, the png should as well. In PNG tests, we should thus only ensure that 1) the worker doesn't throw an error, and 2) the returned PNG is valid:

I think that's perfectly fine 👍🏻

Also, I was looking at the SVG generation, and since we are taking the content of #container, which is always present, but don't check if it has an svg child, it means an invalid graph (with syntax error) returns a 200 OK for /svg (but fails for /png).

As far as I understand, Mermaid will still generate an SVG image, for instance: https://kroki.io/mermaid/svg/eNpLL0osyFAIceGK5QIAFwMDPA==

Do you have an example where Mermaid does not generate an SVG?

We could change it by adding those lines before the if(isPng) in the worker, so we actually get a 400 BAD REQUEST on invalid syntax:

In any case, I think you're right, it would be better to check if it has an SVG child 👍🏻
If you have a failing diagram at hand, please feel free to add another test case.

Finally, I am wondering if I should update the alpine image since we are at it, or do another PR for that...

We are using Dependabot to update Alpine images (and more broadly dependencies), see: #882

I had a few issues with Alpine 3.14.0, that's why the pull request is still opened. Alpine 3.14.2 is now out but I want to test it thoroughly to make sure that everything is now running fine.

I hope I answered your questions, if not, feel free to ping me 😄

[derlin]

Ok, so I can actually make the code return the error SVG as image, but since this error as SVG doesn't provide any information (except that a problem has occurred), to me it would make sense to instead return 400: BAD REQUEST, with a body Syntax Error in Graph. This way, if you use it from the command-line for example, you can directly know there is a problem without having to actually open the output file (svg or png).

This is actually what most other endpoints are doing: PlantUML, BlockDiag, PacketDiag, etc.
If this is ok with you, I will change the behavior to 400: BAD REQUEST for both endpoints, and add a test about it. Would this be ok ?

[ggrossetie]

This is actually what most other endpoints are doing: PlantUML, BlockDiag, PacketDiag, etc.
If this is ok with you, I will change the behavior to 400: BAD REQUEST for both endpoints, and add a test about it. Would this be ok ?

Yes, that would be more consistent 👍🏻
It's important to note that the SVG does not provide any additional information, it only says:

Syntax error in graph
mermaid version 8.13.2

So, we won't lose information.

Sorry for the late reply, I thought I had replied right when I received the notification (but apparently not). Thanks again for your excellent suggestions 🥇

[ggrossetie]

Perfect, thanks again 👍🏻