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

Previews: support creating thumbnails and full-height screenshots #312

Merged
merged 24 commits into from
Jan 26, 2022

Conversation

ryantxu
Copy link
Member

@ryantxu ryantxu commented Jan 6, 2022

This PR updates the image renderer to support creating thumbnails from a requested page. Rather than adding a new parameter, this is reusing the scale/deviceScaleFactor parameter and using negative numbers.

For the request:
http://localhost:3000/render/d/zNigXIonz/heatmap?kiosk&orgId=1&width=320&height=240&scale=-5

It will render an image a 320*5 and then scale it back down to 320px:

image

@@ -230,10 +248,27 @@ export class Browser {
this.log.debug('Taking screenshot', 'filePath', options.filePath);
}

if (options.fullPageImage) {
this.log.debug('TODO: take full screen image', 'url', options.url);
Copy link
Member Author

Choose a reason for hiding this comment

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

@ArturWierzbicki -- how involved was this process? If easy to port, can we try that?

src/browser/browser.ts Outdated Show resolved Hide resolved
@@ -1,3 +1,6 @@
## 3.4.0 (--)
- Support full height dashboards and scaled thumbnails [#312](https://github.com/grafana/grafana-image-renderer/pull/312)
Copy link
Member Author

Choose a reason for hiding this comment

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

@AgnesToulet what is the process for documenting the changes here?

The behavior changes are:

  1. negative scale will create a scaled down thumbnail
  2. height=-1 will create an image as tall as it needs to be based on the webpage size

Copy link
Contributor

Choose a reason for hiding this comment

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

The documentation for the image renderer is in the grafana repo: https://github.com/grafana/grafana/blob/main/docs/sources/image-rendering but we don't really have documentation on how to call the image renderer (more on how to configure and run it).

I guess we could add a section in the main file or a new file for that. @achatterjee-grafana Do you have any opinion on that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good @AgnesToulet. Let me know if you need any help.

@ryantxu ryantxu changed the title Previews: support creating scaled down images Previews: support creating thumbnails and full-height screenshots Jan 11, 2022
Copy link
Contributor

@AgnesToulet AgnesToulet left a comment

Choose a reason for hiding this comment

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

LGTM! Left a couple of questions and suggestions but all very subjective and nothing major.

Also, I wonder if it would be worth to support the full page option without the dashboard being in kiosk mode (right now, dashboards can be a bit cut at the end without the kiosk mode).

src/browser/browser.ts Outdated Show resolved Hide resolved
src/types.ts Show resolved Hide resolved
src/types.ts Outdated Show resolved Hide resolved
src/config.ts Show resolved Hide resolved
src/browser/browser.ts Outdated Show resolved Hide resolved
@ArturWierzbicki
Copy link
Contributor

@AgnesToulet fixed non-kiosk mode in efa7e73

@ArturWierzbicki
Copy link
Contributor

There is still one issue reported by prebuild-install:

image

prebuild-install WARN This package does not support N-API version v14.18.3 
prebuild-install WARN install No prebuilt binaries found (target=v14.18.3 runtime=napi arch=x64 libc= platform=linux)

Which is weird, because Sharp requires NAPI version >= 5 which is supported by node 14/16: https://nodejs.org/api/n-api.html#n_api_node_api_version_matrix

@ArturWierzbicki
Copy link
Contributor

ArturWierzbicki commented Jan 21, 2022

The only way I found to make it work is to manually copy sharp binary/libvips lib & downgrade pkg to 4.4.9 - which is right before native support for .node files (https://github.com/vercel/pkg/releases/tag/4.5.0). Works on linux, will test on MacOS as well

I also had to pin pkg-fetch dependency of pkg to 2.6.9 due to this issue https://github.com/vercel/pkg-fetch/issues/137. Same fix was introduced in pkg 4.5.1 https://github.com/vercel/pkg/pull/1100/files

@ArturWierzbicki
Copy link
Contributor

Packaging on M1 MacOS doesn't work, neither on this branch (because 2.6.x pkg-fetch doesn't have precompiled arm nodejs) nor on main

@ArturWierzbicki
Copy link
Contributor

Copy link
Contributor

@AgnesToulet AgnesToulet left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the fix for sharp library. Left a few very minor suggestions and one comment to make the scripts work on Windows (the priority is to make it work on Linux but if, with some tweaks, we can make it work on both, it would be great!)

scripts/download_sharp.js Outdated Show resolved Hide resolved
scripts/download_sharp.js Outdated Show resolved Hide resolved
scripts/package_target.sh Outdated Show resolved Hide resolved
scripts/pkg.js Outdated Show resolved Hide resolved
ArturWierzbicki and others added 5 commits January 26, 2022 15:12
Co-authored-by: Agnès Toulet <35176601+AgnesToulet@users.noreply.github.com>
Co-authored-by: Agnès Toulet <35176601+AgnesToulet@users.noreply.github.com>
Co-authored-by: Agnès Toulet <35176601+AgnesToulet@users.noreply.github.com>
@ArturWierzbicki ArturWierzbicki merged commit 614709c into master Jan 26, 2022
@ArturWierzbicki ArturWierzbicki deleted the scaled-thumb-result branch January 26, 2022 19:02
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.

4 participants