Skip to content

S147 — give DLNA a real offset path, and remove S97's clamp in the same commit #574

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.

Sign up for GitHub

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

Merged
detain merged 2 commits into from
Jul 27, 2026
Merged

S147 — give DLNA a real offset path, and remove S97's clamp in the same commit #574

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ba653ee
S147 — give DLNA a real offset path, and remove S97's clamp in the sa…
detain Jul 27, 2026
0ff8be8
S147 review fixes — pin the two COUNTs, restore sort-before-page, cou…
detain Jul 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 59 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -672,6 +672,65 @@ and the project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.

### Fixed

- **DLNA advertised far more children than it could deliver on EVERY root, and none
of them could be paged** (S147). `LibraryBridge::getLibraryChildCount()` counted
with the unbounded `ItemRepository::countAllByType()` while
`getLibraryItems()` listed with `getAllByType()`'s default `LIMIT 100`, and
`ContentDirectory::browseChildren()` then applied the renderer's `StartingIndex`
**in PHP to that already-truncated list** — so any index past the truncation
point returned an empty page while `TotalMatches` still advertised the whole
container. Measured read-only on production 2026-07-27: Video advertised
**10,718** movies and **26,389** episodes against pages of 100 (**107x** and
**264x**), and the Audio root — whose advertised count S97 had already clamped to
2,000 to stop it lying — left **2,656 of 4,656 artists unreachable by any
navigation**.

`StartingIndex` is now resolved in **SQL** on every branch:
`getLibraryItems()` takes an offset and spends it against the same per-type
counts the container advertises (so a page straddling the movie→series boundary
is contiguous); the `music_*` drill-downs and the `parent_id` drill-down
(`ItemRepository::findByParentPage()`) take one too; and `browseChildren()` no
longer slices — it asks for the requested window and reads `TotalMatches` from a
COUNT that shares the listing's predicate.

**S97's clamp is removed in the same change**, which is the half that makes this
a fix rather than a swap: a renderer told "2,000" stops at 2,000 however good the
paging underneath is, so the advertised count is the true unbounded total again
and `LibraryBridge::MAX_PAGE_ROWS` is a **page size**, not a ceiling.

Ordering across page boundaries is the other half. `getAllByType()` ordered by
`sort_title, name` and `findByParent()` by `name` — neither is unique, and with
ties MySQL arranges the tied rows differently per `LIMIT`+`OFFSET`. Measured on
MySQL 8.0.46 with 400 rows sharing one name, paged 50 at a time: **400 rows
returned, only 372 distinct** — 28 silently repeated, 28 never returned at all.
Both queries now end their `ORDER BY` on the `id` PRIMARY KEY, making the order
total; a real-MySQL test pages a whole container and asserts the concatenated
pages equal the unpaged listing row for row.

`SortCriteria` is honoured **globally** for any container that fits in one page
(`LibraryBridge::MAX_PAGE_ROWS`, 2,000): the whole container is fetched and sorted
before the window is taken, so the criteria decides *which* rows the page contains.
The first revision of this change sorted only the page it had already fetched,
which on a 20-child Video root with `RequestedCount 5` and `-dc:title` returned the
five alphabetically **lowest** titles reversed instead of the five highest — a
regression against the previous sort-then-slice behaviour, now pinned by a test.
Past 2,000 children the sort still applies within the returned page only; making it
global there needs the sort key in every listing's `ORDER BY` **and** a merge across
a category's types, and is deliberately left out rather than trading pageability
for it.

One `Browse` now resolves the category counts **once**. Fetching the page and the
advertised total went through two entry points that each recomputed them, so a
single Video-root browse issued `countAllByType()` six times (`movie, series,
video` twice over) — double the COUNTs on an unauthenticated path, and a window in
which a row written between the two evaluations made the advertised total disagree
with the offsets the listing had already spent. The bridge-less `TotalMatches` is
likewise a `countByParent()` now, instead of re-listing the whole container purely
to measure it.

DLNA remains **off by default and unauthenticated**, so this is a functional gap
closed, not a security change.

- **A retagged music track could NEVER be re-parented, and `rescan` could not reach
the tracks that needed it** (S145). Editing a file's `ALBUM` or `ARTIST` tag left
`music_tracks.album_id` / `artist_id` pointing at the *old* album permanently.
Expand Down
200 changes: 181 additions & 19 deletions src/Dlna/ContentDirectory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,6 +365,52 @@ private function browseMetadata(string $objectId, array $item, string $filter):
/**
* Browse direct children of a container.
*
* ⚠ **`StartingIndex` is resolved in SQL, not by slicing here (S147).** This
* method used to fetch the container's children, count THAT list into
* `TotalMatches`, and `array_slice()` it — but the list it was slicing was
* already truncated upstream (`getAllByType()`'s `LIMIT 100`, or the music
* reader's row cap), so every `StartingIndex` past the truncation point
* returned an empty page while `TotalMatches` still advertised the whole
* container. `getChildren()` now asks for exactly the requested window and
* `TotalMatches` comes from a separate COUNT that shares the listing's
* predicate, so the two agree at every index.
*
* ## What `SortCriteria` is and is NOT guaranteed to do
*
* Measured, not assumed — an earlier revision of this docblock claimed the
* sort behaviour was unchanged by S147 and that was **false for containers
* smaller than the old truncation bound**. Master's order was sort → slice,
* so for a container that fitted inside the pre-S147 fetch the WHOLE
* container was sorted before the window was taken; the first revision of
* S147 took the window first and sorted only that page, which on a 20-child
* Video root with `RequestedCount 5` and `-dc:title` returned the five
* alphabetically LOWEST titles reversed instead of the five highest.
*
* What holds now:
*
* - **Container of at most {@see LibraryBridge::MAX_PAGE_ROWS} children:**
* the sort is GLOBAL. Every child is fetched, sorted, and only then
* windowed, so the criteria decides *which* rows the page contains. That
* is master's semantics restored, and it holds for a strictly wider set of
* containers than master ever managed — master sorted a list already cut
* at `getAllByType()`'s `LIMIT 100` **per type**, so a 150-movie root was
* sorted with 50 movies missing.
* - **Container larger than that:** the sort applies WITHIN the returned
* page only, and `StartingIndex` still selects the rows in the listing's
* own order. Making it global there means carrying the sort key into every
* listing's `ORDER BY` **and** merging across a category's
* {@see LibraryBridge::CATEGORY_TYPES} members, because a root is the
* CONCATENATION of its types — sorting each type's SQL and concatenating
* yields movies-desc-then-series-desc, which is not a globally sorted page.
* That is a distinct piece of work; it is not folded in here, and no
* renderer this server has been driven by sends a `SortCriteria` at all.
*
* ⚠ Cost, stated rather than hidden: a sorted browse of a container LARGER
* than one page resolves the data layer twice — once to learn it does not
* fit, once for the real window. Only that combination pays it; the unsorted
* path (every browse this server has actually served) resolves once, which is
* itself a halving of what it cost before this round.
*
* @return array<string, mixed>
*/
private function browseChildren(
Expand All @@ -374,35 +420,99 @@ private function browseChildren(
int $requestedCount,
string $sortCriteria
): array {
// Get children based on object ID type
$children = $this->getChildren($objectId);
$startingIndex = max(0, $startingIndex);

// Sort if needed
if (!empty($sortCriteria)) {
$children = $this->sortItems($children, $sortCriteria);
if ($sortCriteria === '') {
// One window, one count, one categoryTypeCounts() — see childPage().
$page = $this->childPage($objectId, $startingIndex, $requestedCount);

return $this->childrenResult($page['items'], $page['total']);
}

$this->totalMatches = count($children);
// Sorted: take the whole container FIRST when it fits in one page, so the
// criteria selects which rows the window contains rather than merely how
// the window is arranged.
$whole = $this->childPage($objectId, 0, LibraryBridge::MAX_PAGE_ROWS);
if ($whole['total'] <= LibraryBridge::MAX_PAGE_ROWS) {
$sorted = $this->sortItems($whole['items'], $sortCriteria);

// Apply pagination
$resultItems = array_slice($children, $startingIndex, $requestedCount > 0 ? $requestedCount : null);
return $this->childrenResult(
array_slice($sorted, $startingIndex, $requestedCount > 0 ? $requestedCount : null),
$whole['total']
);
}

$didl = $this->generateDidl($resultItems);
// Past one page a global sort is not expressible here (see the docblock).
// The container stays PAGEABLE, which matters more: sorting the page is a
// residual wrong, returning empty pages past row 2,000 is the S97 defect.
$page = $this->childPage($objectId, $startingIndex, $requestedCount);

return $this->childrenResult($this->sortItems($page['items'], $sortCriteria), $page['total']);
}

/**
* One page of a container together with the container's TOTAL, resolved in a
* single pass over the data layer.
*
* @param string $objectId The container to page.
* @param int $startingIndex DLNA `StartingIndex`.
* @param int $requestedCount DLNA `RequestedCount`; 0 means "as many as you
* can", answered with {@see LibraryBridge::MAX_PAGE_ROWS}.
* @return array{items: array<int, array<string, mixed>>, total: int}
*/
private function childPage(string $objectId, int $startingIndex, int $requestedCount): array
{
if ($this->libraryBridge !== null) {
// The object's type is handed over rather than re-discovered:
// `browse()` has ALREADY resolved and cached this row in order to
// choose BrowseMetadata vs BrowseDirectChildren.
$resolved = $this->resolveObjectId($objectId);
$resolvedType = is_string($resolved['type'] ?? null) ? $resolved['type'] : null;

return $this->libraryBridge->browsePage(
$objectId,
$resolvedType,
max(0, $startingIndex),
$requestedCount > 0 ? $requestedCount : LibraryBridge::MAX_PAGE_ROWS
);
}

return [
'Result' => $didl,
'NumberReturned' => count($resultItems),
'TotalMatches' => $this->totalMatches,
'items' => $this->getChildren($objectId, $startingIndex, $requestedCount),
'total' => $this->bridgelessChildCount($objectId),
];
}

/**
* The `Browse` envelope, so the two windowing branches above cannot drift
* apart on what they report.
*
* @param array<int, array<string, mixed>> $items The page being returned.
* @param int $total The container's total child count.
* @return array<string, mixed>
*/
private function childrenResult(array $items, int $total): array
{
$this->totalMatches = $total;

return [
'Result' => $this->generateDidl($items),
'NumberReturned' => count($items),
'TotalMatches' => $total,
'UpdateID' => $this->systemUpdateId,
];
}

/**
* Get children of a container.
*
* @param string $objectId The container to list.
* @param int $startingIndex DLNA `StartingIndex`.
* @param int $requestedCount DLNA `RequestedCount`; 0 means "as many as you
* can", which the bridge answers with {@see LibraryBridge::MAX_PAGE_ROWS}.
* @return array<int, array<string, mixed>>
*/
private function getChildren(string $objectId): array
private function getChildren(string $objectId, int $startingIndex = 0, int $requestedCount = 0): array
{
// Use LibraryBridge if available for real data.
//
Expand All @@ -416,7 +526,12 @@ private function getChildren(string $objectId): array
$resolved = $this->resolveObjectId($objectId);
$resolvedType = is_string($resolved['type'] ?? null) ? $resolved['type'] : null;

return $this->libraryBridge->getContainerChildren($objectId, $resolvedType);
return $this->libraryBridge->getContainerChildren(
$objectId,
$resolvedType,
max(0, $startingIndex),
$requestedCount > 0 ? $requestedCount : LibraryBridge::MAX_PAGE_ROWS
);
}

// NOTE: two placeholder branches used to sit here, dispatching to
Expand All @@ -431,14 +546,61 @@ private function getChildren(string $objectId): array
// discard it. Bridge-less callers now fall through to real container
// resolution below.

// Handle item-based containers
// Handle item-based containers. This is the BRIDGE-LESS path only —
// tests and any caller that builds a ContentDirectory directly — and it
// keeps the in-PHP window because there is no paging primitive behind it
// and nothing here is production-scale. Behaviour is byte-identical to
// what browseChildren() used to do for it.
$children = $this->bridgelessChildren($objectId);

return array_slice($children, max(0, $startingIndex), $requestedCount > 0 ? $requestedCount : null);
}

/**
* Children of a container when NO {@see LibraryBridge} is wired.
*
* @return array<int, array<string, mixed>>
*/
private function bridgelessChildren(string $objectId): array
{
$parentId = $this->bridgelessParentId($objectId);

return $parentId === null ? [] : $this->itemRepository->findByParent($parentId);
}

/**
* `TotalMatches` for the bridge-less path — a COUNT, not the length of a
* second copy of the list.
*
* This used to be `count($this->bridgelessChildren($objectId))`, which
* materialised the whole (unbounded) child list a second time purely to
* measure it, immediately after `getChildren()` had already built it. It
* shares `bridgelessChildren()`'s predicate exactly — same container guard,
* same `parent_id` — so the promise and the deliverable still agree.
*/
private function bridgelessChildCount(string $objectId): int
{
$parentId = $this->bridgelessParentId($objectId);

return $parentId === null ? 0 : $this->itemRepository->countByParent($parentId);
}

/**
* The `parent_id` a bridge-less drill-down reads, or NULL when `$objectId` is
* not a container at all.
*
* One definition so the listing and the count above cannot diverge.
*/
private function bridgelessParentId(string $objectId): ?string
{
$item = $this->resolveObjectId($objectId);
if ($item !== null && ($item['type'] ?? '') === 'container') {
$parentId = $item['id'] ?? '';
return $this->itemRepository->findByParent(is_string($parentId) ? $parentId : '');
if ($item === null || ($item['type'] ?? '') !== 'container') {
return null;
}

return [];
$parentId = $item['id'] ?? '';

return is_string($parentId) ? $parentId : '';
}

/**
Expand Down
Loading
Loading