imageUtils rewrite #769

heyhippari · 2021-02-20T19:32:08Z

Rewrites the utility functions to get image URLs, based on Jellyfin-web.

Allows for image-type preference and proper fallback (For example, Thumbs will be preferred for thumb-shaped cards, but fallback to backdrops, then primary images)
Properly handles parent items
Adds blurhash + fallback for artist and person details page

Thumbs with Backdrop fallback

Proper episode images

sonarcloud · 2021-02-20T19:33:23Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
11 Code Smells

No Coverage information
13.2% Duplication

codecov · 2021-02-20T19:34:13Z

Codecov Report

Merging #769 (389b8c8) into master (0214efb) will decrease coverage by 1.07%.
The diff coverage is 17.14%.

@@            Coverage Diff             @@
##           master     #769      +/-   ##
==========================================
- Coverage   68.44%   67.36%   -1.08%     
==========================================
  Files          24       24              
  Lines         786      806      +20     
  Branches      141      144       +3     
==========================================
+ Hits          538      543       +5     
- Misses        228      243      +15     
  Partials       20       20

Impacted Files	Coverage Δ
client/store/tvShows.ts	`40.00% <ø> (ø)`
client/utils/playbackUtils.ts	`36.84% <ø> (ø)`
client/mixins/itemHelper.ts	`34.21% <7.14%> (-5.48%)`	⬇️
client/utils/items.ts	`88.88% <57.14%> (-7.67%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0214efb...389b8c8. Read the comment docs.

client/components/Item/PersonList.vue

client/mixins/imageHelper.ts

ferferga · 2021-04-04T16:00:46Z

client/pages/artist/_itemId/index.vue

+                v-if="item.ImageTags && item.ImageTags.Primary"
+                :item="item"
+              />
+              <v-icon v-else size="128" dark>mdi-account</v-icon>


Why not do this fallback on BlurhashImage like all the items does?

BlurhashImage also has slots now, no need for v-else

Mostly because this happens:

576fdf1 fixes this by using slots properly in blurhash-image (There's no need to make a condition, you define the default content by putting it into the slot)

I don't have a clue why this is happening. Maybe we can make the v-if here an v-if-else? I don't have a idea why this is happening, we're already getting artist images with proper icons in the home header.

I originally placed the condition so BlurhashImage is smart enough to provide icons for fallbacks but we're still able to pass our own custom slots in case we needed a different component or other needs.

You shouldn't need those slots in any case, I'm testing your changes locally to see what could be going on.

Okay, something strange iis going on. Master is showing me the mid-account icon for artists that don't have an image in the Home Header. Your PR shows the mdi-disc icon instead.

Artists that have images don't have any overlapping icon, so I think you should remove the passed slots altogether and let BlurhashImage handle it like we do in HomeHeader's master and artist cards.

I don't have a clue why this is happening. Maybe we can make the v-if here an v-if-else? I don't have a idea why this is happening, we're already getting artist images with proper icons in the home header.

That part of the code is completely gone.

The way you define a slot's default content isn't with v-if and v-else, but by putting the default inside of the slot.

You shouldn't need those slots in any case, I'm testing your changes locally to see what could be going on.

I removed the ones for the main artist and person images.

However one still remains in PersonList. In order to remove it, we should rework getShapeFromItemType to take BaseItemDto | BaseItemPerson. But that touches stuff outside of this PR's scope, so I'd rather do it in a followup.

Okay, something strange iis going on. Master is showing me the mid-account icon for artists that don't have an image in the Home Header. Your PR shows the mdi-disc icon instead.

It has always showed mdi-disc in the home header for me, since the home header lists albums, not persons.

client/pages/artist/_itemId/index.vue

client/pages/person/_itemId/index.vue

client/utils/items.ts

ferferga · 2021-04-04T16:04:33Z

client/components/Layout/Images/BlurhashImage.vue

      <v-fade-transition>
        <img
          v-show="!loading"
-          :key="`blurhashimage-${item.Id}`"


How does this work when the component is not remounted? (I.e item update or audio controls)

It works just fine for me.

neopc10 · 2021-04-04T17:56:49Z

Would this PR be a good opportunity to include usage of Primary Poster images for the latest media in a mixed library? I know those libraries have been largely untouched but for cosmetics this would go a long way.

heyhippari · 2021-04-04T18:29:48Z

Would this PR be a good opportunity to include usage of Primary Poster images for the latest media in a mixed library? I know those libraries have been largely untouched but for cosmetics this would go a long way.

The logic in this is the exact same as what the web client does, so whatever it uses for images is what this uses too. If that's not the case for mixed libraries, there may be changes needed elsewhere, which would probably fall under another PR :/

neopc10 · 2021-04-04T21:11:46Z

Web is using thumb images. Movies have always used Primary and TV Shows in the past few versions moved to Primary but the mixed were left behind.

heyhippari · 2021-04-04T22:35:03Z

@neopc10 I fixed the shape issue in 80d2bdf. The images all use the Primary, like other sections do (See the screenshot below)

ThibaultNocchi

LGTM

ferferga

I confirm that there aren't issues while switching between tracks in audiocontrols (with a caveat, see review comments).

The issues with people (only tested music artists) I commented in my other review persist though:

Master

This branch

(We expect album covers now or artist images?)

Also, in some places artists has images and in some other they don't:

However, in other item "You may also like":

There are some other cases where the image is present in the library cards but not in the artist page:

I found this happens when you're navigating from a place where the image doesn't exist as well. So, if I click in the library card, I can see the image. However, if I find that artist through "You may also like", there are no images. Same applies with home header and I guess to every place where the image isn't displayed correctly. Following links in albums to artist also reproduce this problem

ferferga · 2021-04-07T10:31:47Z

client/components/Item/PersonList.vue

+              <template #placeholder>
+                <v-icon size="16">mdi-account</v-icon>
+              </template>


Why we need this? If you want to override the sizing, you can pass iconSize prop to BlurhashImage, the rest (component type being icon, icon that will be used) is exactly the same output that BlurhashImage will generate for you.

Because this doesn't use BaseItemDto, but BaseItemPerson.

@MrTimscampi Shouldn't we then instruct BlurhashImage to handle those? Right now they don't have a blurhash as well because this. As you moved everything to a single function in imageHelper I guess this should be possible without much hassle.

I'd rather we get it merged without this as the placeholder mess is very difficult to maintain and having everything centered (like image rescaling on resize, the fallbacks and so on) in blurhashimage turned out to be great in the long term (this PR would have been massive for instance :P)

ferferga · 2021-04-07T10:34:50Z

client/components/Item/RelatedItems.vue

+              <v-avatar color="card">
+                <blurhash-image :item="relatedItem" icon-size="16" />
+              </v-avatar>


Above you want exactly what you do here

ferferga · 2021-04-07T10:37:32Z

client/components/Layout/Images/BlurhashImage.vue

-        this.onError();
-      }
+        this.image = imageInfo.url;
+        this.tag = imageInfo.tag ? imageInfo.tag : 'no-image-tag';


Why not set it to null instead?

Holdover from jf-web, thanks for catching it 😋

ferferga · 2021-04-07T10:40:28Z

client/components/Players/PlayerManager.vue

-                  maxWidth: 96
-                }) || '',
+                this.getImageInfo(this.getCurrentItem, {
+                  width: 96


Maybe we should define in all these functions the height to be 100% sure that the size we get is the one we really expect?

The server won't cut them if we specify the height too, as far as I'm aware. It will just try to make the image fit in the box you send it.

ferferga · 2021-04-07T10:43:30Z

client/mixins/imageHelper.ts

+
+      return aspectRatio;
+    },
+    getImageInfo(


Why all the documentation for this function is completely gone? I think it was really useful, as the image logic is getting more and more complex everyday

ferferga · 2021-04-07T11:01:11Z

client/components/Layout/Images/BlurhashImage.vue

-        :punch="punch"
-        class="absolute"
-      />
+  <div ref="imageElement">


Shouldn't we use better the img element to pass it to the function?

After testing what I comment on #769 (comment) both in your branch and in master, I see that the Cast & Crew images have a far superior resolution in your branch than in master, which makes me suspect that I was indeed right about this change.

Maybe, being the child absolute positioned, the size can't be determined correctly and it's loading full resolution?

That wasn't working, probably because the image doesn't contain anything and is absolute.

This element has the same size anyway, and is technically our image component.

ferferga · 2021-04-07T11:05:11Z

client/components/Layout/Images/BlurhashImage.vue

+      <v-fade-transition>
+        <blurhash-canvas
+          v-if="hash"
+          :hash="hash"
+          :width="width"
+          :height="height"
+          :punch="punch"
+          class="absolute"
+        />
+      </v-fade-transition>


We don't need a v-fade transition here. On fast connections the images load so fast that the transition is longer than the time that it takes to load the connection, so we want to show the blurhash as soon as possible without any fancy thing. The image is another thing though.

With your branch, the transition when switching tracks is very "abrupt" if that makes sense.

ferferga · 2021-04-07T11:05:49Z

client/components/Layout/Images/BlurhashImage.vue

-    <div v-if="!$slots.placeholder" class="absolute icon img">
-      <v-icon
-        class="absolute text--disabled"
-        :size="iconSize"
-        color="white"
-        dark
-      >
+    <slot v-else name="placeholder">
+      <v-icon class="absolute text--disabled" :size="iconSize">
        {{ getItemIcon(item) }}
      </v-icon>
-    </div>
-    <slot v-else name="placeholder" />
+    </slot>


What if somewhere we don't want an icon placeholder but something else instead?

You just use the slot and set whatever you want inside of it.

This is how slots work. What is inside the slot definition is the default. Then you override it by defining a template for that slot when calling the component.

heyhippari · 2021-04-07T17:08:37Z

The issues with people (only tested music artists) I commented in my other review persist though

If fixed this locally, but it's important to note a few things:

We're listing albums and not artists, so the disc makes more sense
It's only showing a person because HomeHeaderItem is currently fetching the parent items. With this PR, this isn't necessary at all (as pointed out before, items have all the necessary tags and blurhashes), so the next step after this is completely removing the related items thing, which will lead to having discs there anyway.

Also, in some places artists has images and in some other they don't

I am not able to reproduce this using a 10.7.1 server and this branch.

There are some other cases where the image is present in the library cards but not in the artist page

Same thing, I'm not able to reproduce it.

I found this happens when you're navigating from a place where the image doesn't exist as well. So, if I click in the library card, I can see the image. However, if I find that artist through "You may also like", there are no images. Same applies with home header and I guess to every place where the image isn't displayed correctly. Following links in albums to artist also reproduce this problem

Doing this gives me images all the time:

Home header => View details => By { artist }
Image shows up
Library => Click on card => By { artist }
Image shows up
Library => Click on artist link on card
Image shows up
On artist page => You may also like
Image shows up

…preference and parents properly

Mixed libraries do not have a CollectionType property, for some reason. As such, they used square cards, which is not correct. This switches the default shape to portrait, which fixes the issue.

…f an item

client/mixins/imageHelper.ts

sonarcloud · 2021-04-09T09:39:38Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
16 Code Smells

No Coverage information
1.0% Duplication

heyhippari mentioned this pull request Feb 20, 2021

First release blockers #557

Closed

16 tasks

heyhippari mentioned this pull request Feb 27, 2021

Add an item store #764

Merged

heyhippari mentioned this pull request Mar 13, 2021

Feat: item store implementation #834

Merged

jellyfin-bot added the merge conflict Something has merge conflicts label Mar 23, 2021

heyhippari force-pushed the image-refactor branch from fdb78b4 to 4ab6186 Compare April 4, 2021 15:43

jellyfin-bot added vue Pull requests that edit or add Vue files pinia Pull requests or issues that deal with Pinia stores labels Apr 4, 2021

heyhippari removed the merge conflict Something has merge conflicts label Apr 4, 2021

heyhippari marked this pull request as ready for review April 4, 2021 15:45

heyhippari force-pushed the image-refactor branch from 4ab6186 to 0a32806 Compare April 4, 2021 15:45

ferferga requested changes Apr 4, 2021

View reviewed changes

jellyfin-bot added the merge conflict Something has merge conflicts label Apr 6, 2021

ThibaultNocchi approved these changes Apr 7, 2021

View reviewed changes

heyhippari force-pushed the image-refactor branch from 15df72e to 0446328 Compare April 7, 2021 07:56

heyhippari requested a review from ferferga April 7, 2021 07:59

ferferga requested changes Apr 7, 2021

View reviewed changes

heyhippari force-pushed the image-refactor branch 2 times, most recently from 81fa362 to 0612997 Compare April 7, 2021 17:34

heyhippari added 5 commits April 9, 2021 10:43

refactor(blurhashimage): rewrite blurhash image to handle image type …

4f2cea0

…preference and parents properly

fix: address review comments

68c767c

fix(home): change default collection shape to portrait

59dcd81

Mixed libraries do not have a CollectionType property, for some reason. As such, they used square cards, which is not correct. This switches the default shape to portrait, which fixes the issue.

fix: fix test and remove superfluous image placeholders

824332e

fix: fix isPerson type guard not always properly detecting the type o…

4fff74f

…f an item

heyhippari added 2 commits April 9, 2021 10:43

refactor(imagehelper): rename getImageUrl to getImageInfo

b8017f1

fix: address comments

efb00a4

heyhippari force-pushed the image-refactor branch from 0612997 to f77b8bd Compare April 9, 2021 09:29

heyhippari removed the merge conflict Something has merge conflicts label Apr 9, 2021

ferferga reviewed Apr 9, 2021

View reviewed changes

client/mixins/imageHelper.ts Outdated Show resolved Hide resolved

docs(imagehelper): add JSDOC for getImageInfo

389b8c8

heyhippari force-pushed the image-refactor branch from f77b8bd to 389b8c8 Compare April 9, 2021 09:38

ferferga approved these changes Apr 9, 2021

View reviewed changes

heyhippari merged commit cf0649b into jellyfin:master Apr 9, 2021

ferferga mentioned this pull request Apr 10, 2021

related items are updated too frequently #990

Closed

imageUtils rewrite #769

imageUtils rewrite #769

Conversation

heyhippari commented Feb 20, 2021 • edited Loading

Thumbs with Backdrop fallback

Proper episode images

sonarcloud bot commented Feb 20, 2021

codecov bot commented Feb 20, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neopc10 commented Apr 4, 2021

heyhippari commented Apr 4, 2021

neopc10 commented Apr 4, 2021

heyhippari commented Apr 4, 2021

ThibaultNocchi left a comment

Choose a reason for hiding this comment

ferferga left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ferferga Apr 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heyhippari commented Apr 7, 2021

sonarcloud bot commented Apr 9, 2021

heyhippari commented Feb 20, 2021 •

edited

Loading

codecov bot commented Feb 20, 2021 •

edited

Loading

ferferga left a comment •

edited

Loading

ferferga Apr 7, 2021 •

edited

Loading