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

Universal viewer (UV) fails rendering png with transparent alpha channel #3760

Merged
merged 6 commits into from
Aug 8, 2019

Conversation

ghost
Copy link

@ghost ghost commented Apr 26, 2019

Fixes #3422; required a new version of Riiif: #125 released as 2.1.

This PR, combined with a change in Riiif will fix an issue whereby images with an alpha channel are incorrectly rendered by Universal Viewer backed by Riiif.

The incorrect rendering occurs because Riiif is asked for a 'jpg', and jpegs do not support alpha channels.

One complication is that Universal Viewer requests 'jpg' even if png is specified in the manifest. The changes proposed in Riiif help to get around that by returning a png if an alpha channel present, irrespective of the request format.

Changes in Hyrax are:

  1. Extend the characterize job to run Imagemagick identify and extract channel information (via the Riiif::ImageInfoExtractor) ... we need channel information to pass to Riiif, and we cannot get this from FITS AFAIK. Channel information could also be useful for preservation purposes, so it is useful to collect it. This channel information is added to the characterization_proxy and indexed into solr.

  2. Change the Riiif info_service to return format AND alpha_channels; Riiif will then check for an alpha channel and preserve this, returning a png instead of a jpg.

@samvera/hyrax-code-reviewers

@stale
Copy link

stale bot commented May 26, 2019

This issue has been automatically marked as stale because it has not had activity for 30 days. It will be closed if no further activity occurs within 14 days. Thank you for your contributions.

@stale stale bot added the stale label May 26, 2019
@cjcolvar
Copy link
Member

cjcolvar commented Jun 3, 2019

I wonder if it will become a problem having this property called "channels" when audio files have "channels" as well.
https://github.com/samvera/hydra-works/blob/master/lib/hydra/works/characterization/schema/audio_schema.rb#L4

@stale stale bot removed the stale label Jun 3, 2019
@ghost
Copy link
Author

ghost commented Jun 3, 2019

@cjcolvar good point, I'll rename it alphachannels ... I want to get this finished this week if at all possible

@cjcolvar
Copy link
Member

cjcolvar commented Jun 3, 2019

It would be good for someone else to review this because I don't work with images and don't feel confident enough.

@ghost
Copy link
Author

ghost commented Jun 3, 2019

@cjcolvar thanks! I need to get the tests passing, and do a bit of manual QA of my own, then I'll ping a couple of reviewers

@ghost ghost force-pushed the 3422_part_two branch 3 times, most recently from e7cf770 to b113c78 Compare June 4, 2019 07:26
@ghost ghost changed the title WIP Universal viewer (UV) fails rendering png with transparent alpha channel Universal viewer (UV) fails rendering png with transparent alpha channel Jun 4, 2019
orangewolf
orangewolf previously approved these changes Jun 4, 2019
Copy link
Member

@orangewolf orangewolf left a comment

Choose a reason for hiding this comment

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

This looks good to me.

… also bumps the circle ci cache_key version to rebuild the test app
@ghost
Copy link
Author

ghost commented Jun 20, 2019

@no-reply Hi Tom, any chance of a review?

Copy link
Member

@cjcolvar cjcolvar left a comment

Choose a reason for hiding this comment

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

👍 Looks good. Thanks @geekscruff and @dlpierce !

@@ -33,7 +33,7 @@ def mount_route

def override_image_url_builder_in_hyrax_config
insert_into_file 'config/initializers/hyrax.rb', before: /^ # config.iiif_image_url_builder/ do
" config.iiif_image_url_builder = lambda do |file_id, base_url, size|\n" \
" config.iiif_image_url_builder = lambda do |file_id, base_url, size, format|\n" \
Copy link
Member

Choose a reason for hiding this comment

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

Does this change in the lambda signature require updating documentation or at least need to be called out in the 3.0 migration docs?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I believe it will.

@dlpierce dlpierce merged commit 83dc1ea into master Aug 8, 2019
@dlpierce dlpierce deleted the 3422_part_two branch August 8, 2019 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

uv fails rendering png with transparent alpha channel
4 participants