Handle content length being null in StreamClient#939
Handle content length being null in StreamClient#939mskovacic wants to merge 1 commit intoTyrrrz:masterfrom
Conversation
Allow downloading videos of unknown content length. That is necessary in scenarios when we the videos are being streamed.
|
Have you tested it? This still wouldn't be enough to be able to download live videos. |
I don't know specifically about live videos, but it solved my problem with this https://www.youtube.com/watch?v=BqvQwOPE3n4 |
There was a problem hiding this comment.
Pull request overview
This PR aims to allow stream manifests (and subsequent downloads) to include streams whose content length cannot be determined from metadata/headers, which is important for streamed/live scenarios where Content-Length may be missing.
Changes:
- Stop skipping streams when
TryGetContentLengthAsync(...)returnsnull. - Introduce a
fileSizevariable and pass it through toMuxedStreamInfo,VideoOnlyStreamInfo, andAudioOnlyStreamInfo.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var contentLength = await TryGetContentLengthAsync(streamData, url, cancellationToken); | ||
| if (contentLength is null) | ||
| continue; | ||
| var fileSize = contentLength is null ? new FileSize() : new FileSize(contentLength.Value); |
There was a problem hiding this comment.
contentLength being null is now represented as new FileSize() (0 bytes). This is not equivalent to “unknown size” and will break downstream logic that relies on a real size (e.g., MediaStream.Length/segmenting uses streamInfo.Size.Bytes; a 0 length causes reads to terminate immediately and can yield empty downloads). Consider keeping size nullable (API change) or adding an explicit “unknown” representation and updating consumers (MediaStream, progress reporting) to handle unknown length without using 0 as a sentinel, or alternatively derive the length via a range request when Content-Length is absent.
| var contentLength = await TryGetContentLengthAsync(streamData, url, cancellationToken); | ||
| if (contentLength is null) | ||
| continue; | ||
| var fileSize = contentLength is null ? new FileSize() : new FileSize(contentLength.Value); |
There was a problem hiding this comment.
This change introduces new behavior (streams with unknown content length are now included in manifests), but there’s no corresponding coverage to ensure GetManifestAsync includes such streams and DownloadAsync/CopyToAsync still functions when the size cannot be determined (including progress reporting). Please add a test that simulates missing Content-Length and verifies the stream can still be downloaded correctly.
| var contentLength = await TryGetContentLengthAsync(streamData, url, cancellationToken); | ||
| if (contentLength is null) | ||
| continue; | ||
| var fileSize = contentLength is null ? new FileSize() : new FileSize(contentLength.Value); |
There was a problem hiding this comment.
PR description still contains the placeholder Closes #ISSUE_NUMBER. Please link the actual issue (or remove the close directive) so the change is properly tracked and auto-linked on merge.
Tyrrrz
changed the title
Allow downloading videos of unknown content length. That is necessary in scenarios when we the videos are being streamed.
Closes #ISSUE_NUMBER