From 5b3da3bcd754ab21ae777623a1dfa611b70a9f67 Mon Sep 17 00:00:00 2001 From: Shadowghost Date: Thu, 6 Aug 2026 12:42:53 +0200 Subject: Cleanup and simplify query helpers --- .../Item/BaseItemRepository.TranslateQuery.cs | 19 +- .../JellyfinQueryHelperExtensions.cs | 283 +++++++++++---------- 2 files changed, 154 insertions(+), 148 deletions(-) diff --git a/Jellyfin.Server.Implementations/Item/BaseItemRepository.TranslateQuery.cs b/Jellyfin.Server.Implementations/Item/BaseItemRepository.TranslateQuery.cs index 8c0a39fe4c..379f480106 100644 --- a/Jellyfin.Server.Implementations/Item/BaseItemRepository.TranslateQuery.cs +++ b/Jellyfin.Server.Implementations/Item/BaseItemRepository.TranslateQuery.cs @@ -555,7 +555,7 @@ public sealed partial class BaseItemRepository if (filter.ArtistIds.Length > 0) { - baseQuery = baseQuery.WhereReferencedItemMultipleTypes(context, [ItemValueType.Artist, ItemValueType.AlbumArtist], filter.ArtistIds); + baseQuery = baseQuery.WhereReferencedItem(context, [ItemValueType.Artist, ItemValueType.AlbumArtist], filter.ArtistIds); } if (filter.AlbumArtistIds.Length > 0) @@ -586,12 +586,12 @@ public sealed partial class BaseItemRepository if (filter.ExcludeArtistIds.Length > 0) { - baseQuery = baseQuery.WhereReferencedItemMultipleTypes(context, [ItemValueType.Artist, ItemValueType.AlbumArtist], filter.ExcludeArtistIds, true); + baseQuery = baseQuery.WhereReferencedItem(context, [ItemValueType.Artist, ItemValueType.AlbumArtist], filter.ExcludeArtistIds, true); } if (filter.GenreIds.Count > 0) { - baseQuery = baseQuery.WhereReferencedItem(context, ItemValueType.Genre, filter.GenreIds.ToArray()); + baseQuery = baseQuery.WhereReferencedItem(context, ItemValueType.Genre, filter.GenreIds); } if (filter.Genres.Count > 0) @@ -617,7 +617,7 @@ public sealed partial class BaseItemRepository if (filter.StudioIds.Length > 0) { - baseQuery = baseQuery.WhereReferencedItem(context, ItemValueType.Studios, filter.StudioIds.ToArray()); + baseQuery = baseQuery.WhereReferencedItem(context, ItemValueType.Studios, filter.StudioIds); } if (filter.OfficialRatings.Length > 0) @@ -963,17 +963,6 @@ public sealed partial class BaseItemRepository baseQuery = baseQuery.WhereHasAnyProviderIds(filter.HasAnyProviderIds); } - if (filter.HasAnyProviderIds is not null && filter.HasAnyProviderIds.Count > 0) - { - var includeAny = filter.HasAnyProviderIds - .SelectMany(kvp => kvp.Value.Select(v => $"{kvp.Key}:{v}")) - .ToArray(); - if (includeAny.Length > 0) - { - baseQuery = baseQuery.Where(e => e.Provider!.Select(f => f.ProviderId + ":" + f.ProviderValue)!.Any(f => includeAny.Contains(f))); - } - } - if (filter.HasImdbId.HasValue) { baseQuery = filter.HasImdbId.Value diff --git a/src/Jellyfin.Database/Jellyfin.Database.Implementations/JellyfinQueryHelperExtensions.cs b/src/Jellyfin.Database/Jellyfin.Database.Implementations/JellyfinQueryHelperExtensions.cs index fec37ce723..5602ae2052 100644 --- a/src/Jellyfin.Database/Jellyfin.Database.Implementations/JellyfinQueryHelperExtensions.cs +++ b/src/Jellyfin.Database/Jellyfin.Database.Implementations/JellyfinQueryHelperExtensions.cs @@ -14,6 +14,11 @@ namespace Jellyfin.Database.Implementations; /// /// Contains a number of query related extensions. /// +/// +/// Every helper here binds its values through . Values embedded as bare +/// constants are inlined into the SQL as literals, which gives each distinct value its own entry in EF's +/// compiled query cache and its own statement for the database to plan. +/// public static class JellyfinQueryHelperExtensions { private static readonly MethodInfo _containsMethodGenericCache = typeof(Enumerable).GetMethods(BindingFlags.Public | BindingFlags.Static).First(m => m.Name == nameof(Enumerable.Contains) && m.GetParameters().Length == 2); @@ -26,14 +31,63 @@ public static class JellyfinQueryHelperExtensions /// The entity. /// The property type to compare. /// The source query. - /// The list of items to check. + /// The list of items to check. An empty list matches nothing. /// Property expression. /// A Query. - public static IQueryable WhereOneOrMany(this IQueryable query, IList oneOf, Expression> property) + public static IQueryable WhereOneOrMany(this IQueryable query, IReadOnlyList oneOf, Expression> property) { return query.Where(OneOrManyExpressionBuilder(oneOf, property)); } + /// + /// Builds an optimised query expression checking one property against a list of values while maintaining an optimal query. + /// + /// The entity. + /// The property type to compare. + /// The list of items to check. An empty list matches nothing. + /// Property expression. + /// A Query. + public static Expression> OneOrManyExpressionBuilder(this IReadOnlyList oneOf, Expression> property) + { + ArgumentNullException.ThrowIfNull(oneOf); + ArgumentNullException.ThrowIfNull(property); + + var parameter = Expression.Parameter(typeof(TEntity), "item"); + property = ParameterReplacer.Replace, Func>(property, property.Parameters[0], parameter); + + if (oneOf.Count == 0) + { + // Fail closed, and without asking the database to unpack an empty collection to prove it. + return Expression.Lambda>(Expression.Constant(false), parameter); + } + + if (oneOf.Count == 1) + { + var value = Expression.Call( + null, + _efParameterInstruction.MakeGenericMethod(typeof(TProperty)), + Expression.Constant(oneOf[0], typeof(TProperty))); + + return Expression.Lambda>( + typeof(TProperty).IsValueType + ? Expression.Equal(property.Body, value) + : Expression.ReferenceEqual(property.Body, value), + parameter); + } + + var containsMethodInfo = _containsQueryCache.GetOrAdd(typeof(TProperty), static (key) => _containsMethodGenericCache.MakeGenericMethod(key)); + + // Binding the whole collection as one parameter keeps the statement identical for any element + // count, instead of emitting one placeholder per element. + return Expression.Lambda>( + Expression.Call( + null, + containsMethodInfo, + Expression.Call(null, _efParameterInstruction.MakeGenericMethod(oneOf.GetType()), Expression.Constant(oneOf)), + property.Body), + parameter); + } + /// /// Builds a query that checks referenced ItemValues for a cross BaseItem lookup. /// @@ -47,207 +101,170 @@ public static class JellyfinQueryHelperExtensions this IQueryable baseQuery, JellyfinDbContext context, ItemValueType itemValueType, - IList referenceIds, + IReadOnlyList referenceIds, bool invert = false) { - return baseQuery.Where(ReferencedItemFilterExpressionBuilder(context, itemValueType, referenceIds, invert)); + return baseQuery.WhereReferencedItem(context, [itemValueType], referenceIds, invert); } /// - /// Builds a query that checks referenced ItemValues for a cross BaseItem lookup. + /// Builds a query that checks referenced ItemValues of any of the given types for a cross BaseItem lookup. /// /// The source query. /// The database context. - /// The type of item value to reference. + /// The types of item value to reference. /// The list of BaseItem ids to check matches. /// If set an exclusion check is performed instead. /// A Query. - public static IQueryable WhereReferencedItemMultipleTypes( + /// + /// Matching is on CleanName alone. Genre/artist/album etc items do not set an ItemValue of their own + /// type, so the referenced item's Type is never consulted and ids whose names clean to the same value + /// are interchangeable across types. + /// + public static IQueryable WhereReferencedItem( this IQueryable baseQuery, JellyfinDbContext context, - IList itemValueTypes, - IList referenceIds, + IReadOnlyList itemValueTypes, + IReadOnlyList referenceIds, bool invert = false) { - var itemFilter = OneOrManyExpressionBuilder(referenceIds, f => f.Id); - var typeFilter = OneOrManyExpressionBuilder(itemValueTypes, m => m.ItemValue.Type); + ArgumentNullException.ThrowIfNull(context); - // Flat sub-selects + Contains instead of a nested correlated .Any(...Any(...)). + // Flat sub-selects rather than a correlated .Any(...Any(...)). var referencedCleanValues = context.BaseItems - .Where(itemFilter) + .Where(OneOrManyExpressionBuilder(referenceIds, e => e.Id)) .Select(e => e.CleanName); var matchingItemIds = context.ItemValuesMap - .Where(typeFilter) + .Where(OneOrManyExpressionBuilder(itemValueTypes, m => m.ItemValue.Type)) .Where(m => referencedCleanValues.Contains(m.ItemValue.CleanValue)) .Select(m => m.ItemId); - if (invert) - { - return baseQuery.Where(e => !matchingItemIds.Contains(e.Id)); - } - - return baseQuery.Where(e => matchingItemIds.Contains(e.Id)); + return invert + ? baseQuery.Where(e => !matchingItemIds.Contains(e.Id)) + : baseQuery.Where(e => matchingItemIds.Contains(e.Id)); } /// - /// Builds a query expression that checks referenced ItemValues for a cross BaseItem lookup. - /// - /// The database context. - /// The type of item value to reference. - /// The list of BaseItem ids to check matches. - /// If set an exclusion check is performed instead. - /// A Query. - public static Expression> ReferencedItemFilterExpressionBuilder( - this JellyfinDbContext context, - ItemValueType itemValueType, - IList referenceIds, - bool invert = false) - { - // Well genre/artist/album etc items do not actually set the ItemValue of thier specitic types so we cannot match it that way. - /* - "(guid in (select itemid from ItemValues where CleanValue = (select CleanName from TypedBaseItems where guid=@GenreIds and Type=2)))" - */ - - var itemFilter = OneOrManyExpressionBuilder(referenceIds, f => f.Id); - - // Flat sub-selects + Contains instead of a nested correlated .Any(...Any(...)). - var referencedCleanValues = context.BaseItems - .Where(itemFilter) - .Select(e => e.CleanName); - - var matchingItemIds = context.ItemValuesMap - .Where(m => m.ItemValue.Type == itemValueType && referencedCleanValues.Contains(m.ItemValue.CleanValue)) - .Select(m => m.ItemId); - - if (invert) - { - return item => !matchingItemIds.Contains(item.Id); - } - - return item => matchingItemIds.Contains(item.Id); - } - - /// - /// Filters items that match any of the specified (provider name, value) pairs. + /// Filters items that have any of the specified providers, optionally restricted to given values. /// /// The source query. - /// Dictionary mapping provider names to arrays of values to match. + /// Dictionary mapping provider names to values to match. An empty value array matches any value for that provider. /// A filtered query. public static IQueryable WhereHasAnyProviderIds( this IQueryable baseQuery, IReadOnlyDictionary providerIds) { - var providerKeys = providerIds - .SelectMany(kvp => kvp.Value.Select(v => $"{kvp.Key}:{v}")) - .ToList(); - - if (providerKeys.Count == 0) - { - return baseQuery; - } - - return baseQuery.Where(e => e.Provider!.Any(p => providerKeys.Contains(p.ProviderId + ":" + p.ProviderValue))); + return baseQuery.WhereProviderMatch(Flatten(providerIds), false); } /// - /// Filters items that have any of the specified providers. Empty/null values match any value for that provider. + /// Filters items that have any of the specified providers, optionally restricted to a given value. /// /// The source query. - /// Dictionary mapping provider names to optional values. + /// Dictionary mapping provider names to optional values. An empty value matches any value for that provider. /// A filtered query. public static IQueryable WhereHasAnyProviderId( this IQueryable baseQuery, IReadOnlyDictionary providerIds) { - var existenceOnly = providerIds - .Where(e => string.IsNullOrEmpty(e.Value)) - .Select(e => e.Key) - .ToList(); - - var specificValues = providerIds - .Where(e => !string.IsNullOrEmpty(e.Value)) - .Select(e => $"{e.Key}:{e.Value}") - .ToList(); - - if (existenceOnly.Count == 0 && specificValues.Count == 0) - { - return baseQuery; - } - - if (existenceOnly.Count == 0) - { - return baseQuery.Where(e => e.Provider!.Any(p => - specificValues.Contains(p.ProviderId + ":" + p.ProviderValue))); - } - - if (specificValues.Count == 0) - { - return baseQuery.Where(e => e.Provider!.Any(p => existenceOnly.Contains(p.ProviderId))); - } - - // Single EXISTS over Provider with both predicates OR'd, instead of two separate subqueries. - return baseQuery.Where(e => e.Provider!.Any(p => - existenceOnly.Contains(p.ProviderId) || - specificValues.Contains(p.ProviderId + ":" + p.ProviderValue))); + return baseQuery.WhereProviderMatch(providerIds, false); } /// - /// Excludes items that match any of the specified (provider name, value) pairs. + /// Excludes items that have any of the specified providers, optionally restricted to a given value. /// /// The source query. - /// Dictionary mapping provider names to values to exclude. + /// Dictionary mapping provider names to optional values. An empty value excludes any value for that provider. /// A filtered query. public static IQueryable WhereExcludeProviderIds( this IQueryable baseQuery, IReadOnlyDictionary providerIds) { - var excludeKeys = providerIds - .Select(e => $"{e.Key}:{e.Value}") - .ToList(); + return baseQuery.WhereProviderMatch(providerIds, true); + } + + private static IEnumerable> Flatten(IReadOnlyDictionary providerIds) + { + ArgumentNullException.ThrowIfNull(providerIds); - if (excludeKeys.Count == 0) + foreach (var (provider, values) in providerIds) { - return baseQuery; - } + if (values is null || values.Length == 0) + { + yield return new KeyValuePair(provider, string.Empty); + continue; + } - return baseQuery.Where(e => e.Provider!.All(p => !excludeKeys.Contains(p.ProviderId + ":" + p.ProviderValue))); + foreach (var value in values) + { + yield return new KeyValuePair(provider, value); + } + } } /// - /// Builds an optimised query expression checking one property against a list of values while maintaining an optimal query. + /// Matches items against a set of (provider, value) pairs, where an empty value means any value for + /// that provider. Emits a single EXISTS over the provider collection with the predicates OR'd, rather + /// than one subquery per predicate group. /// - /// The entity. - /// The property type to compare. - /// The list of items to check. - /// Property expression. - /// A Query. - public static Expression> OneOrManyExpressionBuilder(this IList oneOf, Expression> property) + private static IQueryable WhereProviderMatch( + this IQueryable baseQuery, + IEnumerable> providerIds, + bool invert) { - var parameter = Expression.Parameter(typeof(TEntity), "item"); - property = ParameterReplacer.Replace, Func>(property, property.Parameters[0], parameter); - if (oneOf.Count == 1) + ArgumentNullException.ThrowIfNull(providerIds); + + var existenceOnly = new List(); + var specificValues = new List(); + foreach (var (provider, value) in providerIds) { - var value = oneOf[0]; - if (typeof(TProperty).IsValueType) + if (string.IsNullOrEmpty(value)) { - return Expression.Lambda>(Expression.Equal(property.Body, Expression.Constant(value)), parameter); + existenceOnly.Add(provider); } else { - return Expression.Lambda>(Expression.ReferenceEqual(property.Body, Expression.Constant(value)), parameter); + specificValues.Add(provider + ":" + value); } } - var containsMethodInfo = _containsQueryCache.GetOrAdd(typeof(TProperty), static (key) => _containsMethodGenericCache.MakeGenericMethod(key)); + if (existenceOnly.Count == 0 && specificValues.Count == 0) + { + return baseQuery; + } - // Always wrap the collection in EF.Parameter so EF Core caches a single compiled plan and reuses it across calls. - return Expression.Lambda>( - Expression.Call( - null, - containsMethodInfo, - Expression.Call(null, _efParameterInstruction.MakeGenericMethod(oneOf.GetType()), Expression.Constant(oneOf)), - property.Body), + var predicate = ProviderPredicate(existenceOnly, specificValues); + + // NOT EXISTS rather than NOT IN: the latter yields no rows at all if the subquery can produce NULL. + return invert + ? baseQuery.Where(e => !e.Provider!.AsQueryable().Any(predicate)) + : baseQuery.Where(e => e.Provider!.AsQueryable().Any(predicate)); + } + + private static Expression> ProviderPredicate( + IReadOnlyList existenceOnly, + IReadOnlyList specificValues) + { + if (specificValues.Count == 0) + { + return existenceOnly.OneOrManyExpressionBuilder(p => p.ProviderId); + } + + if (existenceOnly.Count == 0) + { + return specificValues.OneOrManyExpressionBuilder(p => p.ProviderId + ":" + p.ProviderValue); + } + + var byProvider = existenceOnly.OneOrManyExpressionBuilder(p => p.ProviderId); + var byPair = specificValues.OneOrManyExpressionBuilder(p => p.ProviderId + ":" + p.ProviderValue); + + // Both builders mint their own parameter; rebind so the two bodies can share one lambda. + var parameter = byProvider.Parameters[0]; + var reboundPair = ParameterReplacer.Replace, Func>(byPair, byPair.Parameters[0], parameter); + + return Expression.Lambda>( + Expression.OrElse(byProvider.Body, reboundPair.Body), parameter); } -- cgit v1.2.3 From 9162c178346421275a7d1fc4d2a7b6c3169dbff5 Mon Sep 17 00:00:00 2001 From: Shadowghost Date: Fri, 7 Aug 2026 07:23:50 +0200 Subject: Apply review suggestions --- .../JellyfinQueryHelperExtensions.cs | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/src/Jellyfin.Database/Jellyfin.Database.Implementations/JellyfinQueryHelperExtensions.cs b/src/Jellyfin.Database/Jellyfin.Database.Implementations/JellyfinQueryHelperExtensions.cs index 5602ae2052..0dfce732ce 100644 --- a/src/Jellyfin.Database/Jellyfin.Database.Implementations/JellyfinQueryHelperExtensions.cs +++ b/src/Jellyfin.Database/Jellyfin.Database.Implementations/JellyfinQueryHelperExtensions.cs @@ -24,6 +24,7 @@ public static class JellyfinQueryHelperExtensions private static readonly MethodInfo _containsMethodGenericCache = typeof(Enumerable).GetMethods(BindingFlags.Public | BindingFlags.Static).First(m => m.Name == nameof(Enumerable.Contains) && m.GetParameters().Length == 2); private static readonly MethodInfo _efParameterInstruction = typeof(EF).GetMethod(nameof(EF.Parameter), BindingFlags.Public | BindingFlags.Static)!; private static readonly ConcurrentDictionary _containsQueryCache = new(); + private static readonly ConcurrentDictionary _efParameterCache = new(); /// /// Builds an optimised query checking one property against a list of values while maintaining an optimal query. @@ -65,7 +66,7 @@ public static class JellyfinQueryHelperExtensions { var value = Expression.Call( null, - _efParameterInstruction.MakeGenericMethod(typeof(TProperty)), + EfParameterFor(typeof(TProperty)), Expression.Constant(oneOf[0], typeof(TProperty))); return Expression.Lambda>( @@ -83,11 +84,16 @@ public static class JellyfinQueryHelperExtensions Expression.Call( null, containsMethodInfo, - Expression.Call(null, _efParameterInstruction.MakeGenericMethod(oneOf.GetType()), Expression.Constant(oneOf)), + Expression.Call(null, EfParameterFor(oneOf.GetType()), Expression.Constant(oneOf)), property.Body), parameter); } + private static MethodInfo EfParameterFor(Type type) + { + return _efParameterCache.GetOrAdd(type, static (key) => _efParameterInstruction.MakeGenericMethod(key)); + } + /// /// Builds a query that checks referenced ItemValues for a cross BaseItem lookup. /// @@ -246,16 +252,6 @@ public static class JellyfinQueryHelperExtensions IReadOnlyList existenceOnly, IReadOnlyList specificValues) { - if (specificValues.Count == 0) - { - return existenceOnly.OneOrManyExpressionBuilder(p => p.ProviderId); - } - - if (existenceOnly.Count == 0) - { - return specificValues.OneOrManyExpressionBuilder(p => p.ProviderId + ":" + p.ProviderValue); - } - var byProvider = existenceOnly.OneOrManyExpressionBuilder(p => p.ProviderId); var byPair = specificValues.OneOrManyExpressionBuilder(p => p.ProviderId + ":" + p.ProviderValue); -- cgit v1.2.3 From e120b7f2dd986d7f07f6814b6d987bafd46baab8 Mon Sep 17 00:00:00 2001 From: vavallee Date: Fri, 7 Aug 2026 12:33:37 -0300 Subject: Stop image endpoints from upscaling beyond the source resolution ImageHelper.GetNewImageSize passed the caller-supplied width/height straight through to SkiaEncoder.EncodeImage, which allocates an SKImageInfo of exactly that size. Nothing bounded those values against the source image, so a request like Items//Images/Primary?width=23100&height=23100 made the server allocate and resample a 23100x23100 surface from, say, a 600x336 poster: the reporter measured 100% of a core for 10-15 minutes and 6-12 GB resident per request. The item images endpoints do not require authentication, so any caller who knows an item id can trigger this, and varying the size by one pixel misses the cache every time. Add DrawingUtils.ScaleDownToFit, which scales a size down uniformly until it fits inside a bounding box and returns it unchanged if it already does, and apply it in GetNewImageSize against the original image dimensions. Requests that ask for more pixels than the source now get the source resolution back, scaled to the requested aspect ratio. Downscaling paths are untouched, and DrawingUtils.Resize keeps its existing behaviour for the transcoding callers in EncodingJobInfo and StreamInfo, which legitimately size video output. ResizeFill already refused to upscale; this makes width/height consistent with fillWidth/fillHeight. Fixes #17056. --- MediaBrowser.Controller/Drawing/ImageHelper.cs | 6 +- MediaBrowser.Model/Drawing/DrawingUtils.cs | 29 ++++++++++ .../Drawing/ImageHelperTests.cs | 64 ++++++++++++++++++++++ .../Drawing/DrawingUtilsTests.cs | 28 ++++++++++ 4 files changed, 126 insertions(+), 1 deletion(-) create mode 100644 tests/Jellyfin.Controller.Tests/Drawing/ImageHelperTests.cs create mode 100644 tests/Jellyfin.Model.Tests/Drawing/DrawingUtilsTests.cs diff --git a/MediaBrowser.Controller/Drawing/ImageHelper.cs b/MediaBrowser.Controller/Drawing/ImageHelper.cs index 9ef92bc981..c1d0203897 100644 --- a/MediaBrowser.Controller/Drawing/ImageHelper.cs +++ b/MediaBrowser.Controller/Drawing/ImageHelper.cs @@ -11,7 +11,11 @@ namespace MediaBrowser.Controller.Drawing // Determine the output size based on incoming parameters var newSize = DrawingUtils.Resize(originalImageSize, options.Width ?? 0, options.Height ?? 0, options.MaxWidth ?? 0, options.MaxHeight ?? 0); newSize = DrawingUtils.ResizeFill(newSize, options.FillWidth, options.FillHeight); - return newSize; + + // Never encode larger than the source. Upscaling adds no detail, and the requested + // width/height are caller-controlled, so without this an unauthenticated request can + // pin a CPU and allocate several GB encoding a single image. + return DrawingUtils.ScaleDownToFit(newSize, originalImageSize); } } } diff --git a/MediaBrowser.Model/Drawing/DrawingUtils.cs b/MediaBrowser.Model/Drawing/DrawingUtils.cs index 2040d26bbb..1fdd6a4a49 100644 --- a/MediaBrowser.Model/Drawing/DrawingUtils.cs +++ b/MediaBrowser.Model/Drawing/DrawingUtils.cs @@ -103,6 +103,35 @@ namespace MediaBrowser.Model.Drawing return new ImageDimensions(newWidth, newHeight); } + /// + /// Scales a size down uniformly until it fits inside a bounding box. + /// Returns the original size if it already fits, so this never upscales. + /// + /// The size object. + /// The box the result has to fit inside. + /// A new size object, or if it already fits. + public static ImageDimensions ScaleDownToFit(ImageDimensions size, ImageDimensions boundingBox) + { + if (size.Width <= 0 || size.Height <= 0 || boundingBox.Width <= 0 || boundingBox.Height <= 0) + { + return size; + } + + double widthRatio = size.Width / (double)boundingBox.Width; + double heightRatio = size.Height / (double)boundingBox.Height; + double scaleRatio = Math.Max(widthRatio, heightRatio); + + if (scaleRatio <= 1) + { + return size; + } + + var newWidth = Math.Clamp(Convert.ToInt32(Math.Round(size.Width / scaleRatio)), 1, boundingBox.Width); + var newHeight = Math.Clamp(Convert.ToInt32(Math.Round(size.Height / scaleRatio)), 1, boundingBox.Height); + + return new ImageDimensions(newWidth, newHeight); + } + /// /// Gets the new width. /// diff --git a/tests/Jellyfin.Controller.Tests/Drawing/ImageHelperTests.cs b/tests/Jellyfin.Controller.Tests/Drawing/ImageHelperTests.cs new file mode 100644 index 0000000000..571cb7f0d4 --- /dev/null +++ b/tests/Jellyfin.Controller.Tests/Drawing/ImageHelperTests.cs @@ -0,0 +1,64 @@ +using MediaBrowser.Controller.Drawing; +using MediaBrowser.Model.Drawing; +using Xunit; + +namespace Jellyfin.Controller.Tests.Drawing; + +public static class ImageHelperTests +{ + [Fact] + public static void GetNewImageSize_ExplicitSizeLargerThanSource_ClampsToSource() + { + // Regression test for https://github.com/jellyfin/jellyfin/issues/17056: the caller-supplied + // width/height were used verbatim, so a single request could ask for a 23100x23100 encode. + var options = new ImageProcessingOptions { Width = 23100, Height = 23100 }; + + var newSize = ImageHelper.GetNewImageSize(options, new ImageDimensions(600, 336)); + + Assert.Equal(336, newSize.Width); + Assert.Equal(336, newSize.Height); + } + + [Fact] + public static void GetNewImageSize_WidthLargerThanSource_ClampsToSource() + { + var options = new ImageProcessingOptions { Width = 10000 }; + + var newSize = ImageHelper.GetNewImageSize(options, new ImageDimensions(600, 336)); + + Assert.Equal(600, newSize.Width); + Assert.Equal(336, newSize.Height); + } + + [Fact] + public static void GetNewImageSize_FillLargerThanSource_ClampsToSource() + { + // ResizeFill already refused to upscale; this pins that behaviour. + var options = new ImageProcessingOptions { FillWidth = 23100, FillHeight = 23100 }; + + var newSize = ImageHelper.GetNewImageSize(options, new ImageDimensions(600, 336)); + + Assert.Equal(600, newSize.Width); + Assert.Equal(336, newSize.Height); + } + + [Fact] + public static void GetNewImageSize_SmallerThanSource_StillDownscales() + { + var options = new ImageProcessingOptions { MaxWidth = 300 }; + + var newSize = ImageHelper.GetNewImageSize(options, new ImageDimensions(600, 336)); + + Assert.Equal(300, newSize.Width); + Assert.Equal(168, newSize.Height); + } + + [Fact] + public static void GetNewImageSize_NoSizeRequested_ReturnsSource() + { + var newSize = ImageHelper.GetNewImageSize(new ImageProcessingOptions(), new ImageDimensions(600, 336)); + + Assert.Equal(600, newSize.Width); + Assert.Equal(336, newSize.Height); + } +} diff --git a/tests/Jellyfin.Model.Tests/Drawing/DrawingUtilsTests.cs b/tests/Jellyfin.Model.Tests/Drawing/DrawingUtilsTests.cs new file mode 100644 index 0000000000..473b07a8a1 --- /dev/null +++ b/tests/Jellyfin.Model.Tests/Drawing/DrawingUtilsTests.cs @@ -0,0 +1,28 @@ +using MediaBrowser.Model.Drawing; +using Xunit; + +namespace Jellyfin.Model.Drawing; + +public static class DrawingUtilsTests +{ + [Theory] + // Already inside the box, returned untouched. + [InlineData(600, 336, 1920, 1080, 600, 336)] + [InlineData(1920, 1080, 1920, 1080, 1920, 1080)] + // Scaled down uniformly, requested aspect ratio preserved. + [InlineData(23100, 23100, 1920, 1080, 1080, 1080)] + [InlineData(3840, 2160, 1920, 1080, 1920, 1080)] + [InlineData(1200, 400, 600, 336, 600, 200)] + // Extreme ratios still produce at least one pixel per axis. + [InlineData(10000, 1, 100, 100, 100, 1)] + // Degenerate inputs are passed through rather than dividing by zero. + [InlineData(600, 336, 0, 0, 600, 336)] + [InlineData(0, 0, 1920, 1080, 0, 0)] + public static void ScaleDownToFit_Bounds_WithoutUpscaling(int width, int height, int boxWidth, int boxHeight, int expectedWidth, int expectedHeight) + { + var scaled = DrawingUtils.ScaleDownToFit(new ImageDimensions(width, height), new ImageDimensions(boxWidth, boxHeight)); + + Assert.Equal(expectedWidth, scaled.Width); + Assert.Equal(expectedHeight, scaled.Height); + } +} -- cgit v1.2.3 From c091ffdc6b2d8d4dd6f439d056c561bae7bd9a18 Mon Sep 17 00:00:00 2001 From: brandon Date: Fri, 7 Aug 2026 22:51:45 -0400 Subject: Batch alternate version detection in DtoService to remove MediaSourceCount N+1 Browsing a page of videos with the MediaSourceCount field ran one alternate version query per item, each opening a fresh DbContext. On a large library that turned a single page into hundreds of sequential round trips and made the Items endpoint take tens of seconds while holding a request thread the whole time. Detect which videos own alternate versions once per page with a single query, mirroring the existing people batch. Videos absent from that set have a single media source, so the per item lookups are skipped for the common case. Behavior is unchanged: a video with no alternates already resolved to a count of one. Adds a regression test asserting the count resolves from the batch and the per item lookups are never called. --- Emby.Server.Implementations/Dto/DtoService.cs | 49 ++++++++++++++++------ .../Library/LibraryManager.cs | 6 +++ .../Item/LinkedChildrenService.cs | 21 ++++++++++ MediaBrowser.Controller/Library/ILibraryManager.cs | 8 ++++ .../Persistence/ILinkedChildrenService.cs | 9 ++++ .../Dto/DtoServiceImageInheritanceTests.cs | 42 +++++++++++++++++++ 6 files changed, 123 insertions(+), 12 deletions(-) diff --git a/Emby.Server.Implementations/Dto/DtoService.cs b/Emby.Server.Implementations/Dto/DtoService.cs index da0c52df5b..062c19a1d4 100644 --- a/Emby.Server.Implementations/Dto/DtoService.cs +++ b/Emby.Server.Implementations/Dto/DtoService.cs @@ -253,6 +253,18 @@ namespace Emby.Server.Implementations.Dto } } + // Batch-detect which videos own alternate versions to avoid the per-item alternate-version + // queries in MediaSourceCount. Videos absent from this set have a single media source. + IReadOnlySet? alternateVersionItemIds = null; + if (options.ContainsField(ItemFields.MediaSourceCount)) + { + var versionItemIds = accessibleItems.OfType /// The item IDs to check. /// The set of item IDs that have alternate versions. - IReadOnlySet GetItemsWithAlternateVersions(IReadOnlyList itemIds); + IReadOnlySet GetItemIdsWithAlternateVersions(IReadOnlyList itemIds); /// /// Creates or updates a LinkedChild entry linking a parent to a child item. diff --git a/MediaBrowser.Controller/Persistence/ILinkedChildrenService.cs b/MediaBrowser.Controller/Persistence/ILinkedChildrenService.cs index c1fe3231f4..79c29410e4 100644 --- a/MediaBrowser.Controller/Persistence/ILinkedChildrenService.cs +++ b/MediaBrowser.Controller/Persistence/ILinkedChildrenService.cs @@ -26,7 +26,7 @@ public interface ILinkedChildrenService /// /// The item IDs to check. /// The set of item IDs that have alternate versions. - IReadOnlySet GetItemsWithAlternateVersions(IReadOnlyList itemIds); + IReadOnlySet GetItemIdsWithAlternateVersions(IReadOnlyList itemIds); /// /// Gets all artist matches from the database. diff --git a/tests/Jellyfin.Server.Implementations.Tests/Dto/DtoServiceImageInheritanceTests.cs b/tests/Jellyfin.Server.Implementations.Tests/Dto/DtoServiceImageInheritanceTests.cs index fa94250287..6a3dcab57a 100644 --- a/tests/Jellyfin.Server.Implementations.Tests/Dto/DtoServiceImageInheritanceTests.cs +++ b/tests/Jellyfin.Server.Implementations.Tests/Dto/DtoServiceImageInheritanceTests.cs @@ -222,11 +222,11 @@ public class DtoServiceImageInheritanceTests var libraryManager = new Mock(); // DtoService detects which videos own alternate versions in ONE batch - // (GetItemsWithAlternateVersions) before the per-item loop. Videos absent from that set have a + // (GetItemIdsWithAlternateVersions) before the per-item loop. Videos absent from that set have a // single media source, so the per-item GetLinkedAlternateVersions/GetLocalAlternateVersionIds // queries (the N+1) must be skipped entirely. Here neither movie has alternate versions. libraryManager - .Setup(x => x.GetItemsWithAlternateVersions(It.IsAny>())) + .Setup(x => x.GetItemIdsWithAlternateVersions(It.IsAny>())) .Returns(new HashSet()); var dtoService = BuildDtoService(libraryManager); @@ -236,13 +236,54 @@ public class DtoServiceImageInheritanceTests Assert.Equal(2, dtos.Count); + // A single media source is the default, so the count is left unset (the client treats null as one). + foreach (var dto in dtos) + { + Assert.Null(dto.MediaSourceCount); + } + // The alternate-version check is batched once for the whole set, and the per-item lookups are // never reached because the batch already ruled out alternate versions. - libraryManager.Verify(x => x.GetItemsWithAlternateVersions(It.IsAny>()), Times.Once); + libraryManager.Verify(x => x.GetItemIdsWithAlternateVersions(It.IsAny>()), Times.Once); libraryManager.Verify(x => x.GetLinkedAlternateVersions(It.IsAny