From e691c807741b304ff8df750d4898d3abf3a83ea7 Mon Sep 17 00:00:00 2001 From: Evan Harris Date: Tue, 11 Jun 2019 05:32:10 -0500 Subject: [PATCH 1/6] Change back to allow transcodes to allow seeking This switches back to allowing range rather than chunked requests so that seeking works in the web player, but does so by safer means than previous solutions, by slightly over-estimating the transcoded size, then sending dummy bytes to the client to fill any gap. Fixes #1117. Addresses #685. --- .../player/controller/StreamController.java | 62 ++++++++++++------- 1 file changed, 40 insertions(+), 22 deletions(-) diff --git a/airsonic-main/src/main/java/org/airsonic/player/controller/StreamController.java b/airsonic-main/src/main/java/org/airsonic/player/controller/StreamController.java index 3207af01..b0070fb7 100644 --- a/airsonic-main/src/main/java/org/airsonic/player/controller/StreamController.java +++ b/airsonic-main/src/main/java/org/airsonic/player/controller/StreamController.java @@ -128,6 +128,7 @@ public class StreamController { MediaFile file = getSingleFile(request); boolean isSingleFile = file != null; HttpRange range = null; + Long fileLengthExpected = null; if (isSingleFile) { @@ -153,39 +154,36 @@ public class StreamController { TranscodingService.Parameters parameters = transcodingService.getParameters(file, player, maxBitRate, preferredTargetFormat, null); - boolean isConversion = parameters.isDownsample() || parameters.isTranscode(); - boolean estimateContentLength = ServletRequestUtils.getBooleanParameter(request, - "estimateContentLength", false); boolean isHls = ServletRequestUtils.getBooleanParameter(request, "hls", false); + fileLengthExpected = parameters.getExpectedLength(); // Wrangle response length and ranges. // // Support ranges as long as we're not transcoding; video is always assumed to transcode - if (isConversion || file.isVideo()) { + if (file.isVideo()) { // Use chunked transfer; do not accept range requests response.setStatus(HttpServletResponse.SC_OK); response.setHeader("Accept-Ranges", "none"); } else { // Not transcoding, partial content permitted because we know the final size long contentLength; - // If range was requested, respond in kind - range = getRange(request, file); + range = getRange(request, file.getDurationSeconds(), fileLengthExpected); if (range != null) { response.setStatus(HttpServletResponse.SC_PARTIAL_CONTENT); response.setHeader("Accept-Ranges", "bytes"); // Both ends are inclusive long startByte = range.getFirstBytePos(); - long endByte = range.isClosed() ? range.getLastBytePos() : file.getFileSize() - 1; + long endByte = range.isClosed() ? range.getLastBytePos() : fileLengthExpected - 1; response.setHeader("Content-Range", - String.format("bytes %d-%d/%d", startByte, endByte, file.getFileSize())); + String.format("bytes %d-%d/%d", startByte, endByte, fileLengthExpected)); contentLength = endByte + 1 - startByte; } else { // No range was requested, give back the whole file response.setStatus(HttpServletResponse.SC_OK); - contentLength = file.getFileSize(); + contentLength = fileLengthExpected; } response.setIntHeader("ETag", file.getId()); @@ -212,6 +210,10 @@ public class StreamController { return; } + if (fileLengthExpected != null) { + LOG.info("Streaming request for [{}] with range [{}]", file.getPath(), response.getHeader("Content-Range")); + } + // Terminate any other streams to this player. if (!isPodcast && !isSingleFile) { for (TransferStatus streamStatus : statusService.getStreamStatusesForPlayer(player)) { @@ -230,25 +232,31 @@ public class StreamController { ) { final int BUFFER_SIZE = 2048; byte[] buf = new byte[BUFFER_SIZE]; + long bytesWritten = 0; while (!status.terminated()) { if (player.getPlayQueue().getStatus() == PlayQueue.Status.STOPPED) { if (isPodcast || isSingleFile) { break; } else { - sendDummy(buf, out); + sendDummyDelayed(buf, out); } } else { int n = in.read(buf); if (n == -1) { if (isPodcast || isSingleFile) { + // Pad the output if needed to avoid content length errors on transcodes + if (fileLengthExpected != null && bytesWritten < fileLengthExpected) { + sendDummy(buf, out, fileLengthExpected - bytesWritten); + } break; } else { - sendDummy(buf, out); + sendDummyDelayed(buf, out); } } else { out.write(buf, 0, n); + bytesWritten += n; } } } @@ -340,11 +348,12 @@ public class StreamController { return file.getFileSize(); } - return duration * (long)maxBitRate * 1000L / 8L; + // Over-estimate size a bit (2 seconds) so don't cut off early in case of small calculation differences + return (duration + 2) * (long)maxBitRate * 1000L / 8L; } @Nullable - private HttpRange getRange(HttpServletRequest request, MediaFile file) { + private HttpRange getRange(HttpServletRequest request, Integer fileDuration, Long fileSize) { // First, look for "Range" HTTP header. HttpRange range = HttpRange.valueOf(request.getHeader("Range")); @@ -354,27 +363,25 @@ public class StreamController { // Second, look for "offsetSeconds" request parameter. String offsetSeconds = request.getParameter("offsetSeconds"); - range = parseAndConvertOffsetSeconds(offsetSeconds, file); + range = parseAndConvertOffsetSeconds(offsetSeconds, fileDuration, fileSize); return range; } @Nullable - private HttpRange parseAndConvertOffsetSeconds(String offsetSeconds, MediaFile file) { + private HttpRange parseAndConvertOffsetSeconds(String offsetSeconds, Integer fileDuration, Long fileSize) { if (offsetSeconds == null) { return null; } try { - Integer duration = file.getDurationSeconds(); - Long fileSize = file.getFileSize(); - if (duration == null || fileSize == null) { + if (fileDuration == null || fileSize == null) { return null; } float offset = Float.parseFloat(offsetSeconds); // Convert from time offset to byte offset. - long byteOffset = (long) (fileSize * (offset / duration)); + long byteOffset = (long) (fileSize * (offset / fileDuration)); return new HttpRange(byteOffset, null); } catch (Exception x) { @@ -455,17 +462,28 @@ public class StreamController { return size + (size % 2); } + private void sendDummy(byte[] buf, OutputStream out, long len) throws IOException { + long bytesWritten = 0; + int n; + + Arrays.fill(buf, (byte) 0xFF); + while (bytesWritten < len) { + n = (int) Math.min(buf.length, len - bytesWritten); + out.write(buf, 0, n); + bytesWritten += n; + } + } + /** * Feed the other end with some dummy data to keep it from reconnecting. */ - private void sendDummy(byte[] buf, OutputStream out) throws IOException { + private void sendDummyDelayed(byte[] buf, OutputStream out) throws IOException { try { Thread.sleep(2000); } catch (InterruptedException x) { LOG.warn("Interrupted in sleep.", x); } - Arrays.fill(buf, (byte) 0xFF); - out.write(buf); + sendDummy(buf, out, buf.length); out.flush(); } } From b28ffb0d1ac3c7f475741bbfa76c24c1aab4371c Mon Sep 17 00:00:00 2001 From: Evan Harris Date: Wed, 19 Jun 2019 05:14:58 -0500 Subject: [PATCH 2/6] Added isRangeAllowed() function for transcoded sources This isn't a perfect solution, but it should help increase confidence that the transcoder in use is likely to produce an approximately correct-sized stream. --- .../player/controller/StreamController.java | 6 ++-- .../player/service/TranscodingService.java | 32 +++++++++++++++++++ 2 files changed, 35 insertions(+), 3 deletions(-) diff --git a/airsonic-main/src/main/java/org/airsonic/player/controller/StreamController.java b/airsonic-main/src/main/java/org/airsonic/player/controller/StreamController.java index b0070fb7..050791bb 100644 --- a/airsonic-main/src/main/java/org/airsonic/player/controller/StreamController.java +++ b/airsonic-main/src/main/java/org/airsonic/player/controller/StreamController.java @@ -159,13 +159,13 @@ public class StreamController { // Wrangle response length and ranges. // - // Support ranges as long as we're not transcoding; video is always assumed to transcode - if (file.isVideo()) { + // Support ranges as long as we're not transcoding blindly; video is always assumed to transcode + if (file.isVideo() || ! parameters.isRangeAllowed()) { // Use chunked transfer; do not accept range requests response.setStatus(HttpServletResponse.SC_OK); response.setHeader("Accept-Ranges", "none"); } else { - // Not transcoding, partial content permitted because we know the final size + // Partial content permitted because either know or expect to be able to predict the final size long contentLength; // If range was requested, respond in kind range = getRange(request, file.getDurationSeconds(), fileLengthExpected); diff --git a/airsonic-main/src/main/java/org/airsonic/player/service/TranscodingService.java b/airsonic-main/src/main/java/org/airsonic/player/service/TranscodingService.java index f7bf98dc..a98ca944 100644 --- a/airsonic-main/src/main/java/org/airsonic/player/service/TranscodingService.java +++ b/airsonic-main/src/main/java/org/airsonic/player/service/TranscodingService.java @@ -216,6 +216,7 @@ public class TranscodingService { } parameters.setMaxBitRate(maxBitRate); + parameters.setRangeAllowed(isRangeAllowed(parameters)); return parameters; } @@ -487,6 +488,28 @@ public class TranscodingService { return matches != null && matches.length > 0; } + private boolean isRangeAllowed(Parameters parameters) { + Transcoding transcoding = parameters.getTranscoding(); + List steps = Arrays.asList(); + if (transcoding != null) { + steps = Arrays.asList(transcoding.getStep3(), transcoding.getStep2(), transcoding.getStep1()); + } + else if (parameters.isDownsample()) { + steps = Arrays.asList(settingsService.getDownsamplingCommand()); + } + else { + return true; // neither transcoding or downsampling + } + + // Check if last configured step uses the bitrate, if so, range should be pretty safe + for (String step : steps) { + if (step != null) { + return step.contains("%b"); + } + } + return false; + } + /** * Returns the directory in which all transcoders are installed. */ @@ -517,6 +540,7 @@ public class TranscodingService { public static class Parameters { private boolean downsample; + private boolean rangeAllowed; private final MediaFile mediaFile; private final VideoTranscodingSettings videoTranscodingSettings; private Integer maxBitRate; @@ -543,6 +567,14 @@ public class TranscodingService { return transcoding != null; } + public boolean isRangeAllowed() { + return this.rangeAllowed; + } + + public void setRangeAllowed(boolean rangeAllowed) { + this.rangeAllowed = rangeAllowed; + } + public void setTranscoding(Transcoding transcoding) { this.transcoding = transcoding; } From 6cfbe0ff880506003a135236f7143a67085ab2fb Mon Sep 17 00:00:00 2001 From: Evan Harris Date: Sat, 29 Jun 2019 14:28:39 -0500 Subject: [PATCH 3/6] Moved expected content length calculation to TranscodingService --- .../player/controller/StreamController.java | 23 ---------- .../player/service/TranscodingService.java | 43 ++++++++++++++++++- 2 files changed, 42 insertions(+), 24 deletions(-) diff --git a/airsonic-main/src/main/java/org/airsonic/player/controller/StreamController.java b/airsonic-main/src/main/java/org/airsonic/player/controller/StreamController.java index 050791bb..29dbb31c 100644 --- a/airsonic-main/src/main/java/org/airsonic/player/controller/StreamController.java +++ b/airsonic-main/src/main/java/org/airsonic/player/controller/StreamController.java @@ -329,29 +329,6 @@ public class StreamController { return null; } - private long getFileLength(TranscodingService.Parameters parameters) { - MediaFile file = parameters.getMediaFile(); - - if (!parameters.isDownsample() && !parameters.isTranscode()) { - return file.getFileSize(); - } - Integer duration = file.getDurationSeconds(); - Integer maxBitRate = parameters.getMaxBitRate(); - - if (duration == null) { - LOG.warn("Unknown duration for " + file + ". Unable to estimate transcoded size."); - return file.getFileSize(); - } - - if (maxBitRate == null) { - LOG.error("Unknown bit rate for " + file + ". Unable to estimate transcoded size."); - return file.getFileSize(); - } - - // Over-estimate size a bit (2 seconds) so don't cut off early in case of small calculation differences - return (duration + 2) * (long)maxBitRate * 1000L / 8L; - } - @Nullable private HttpRange getRange(HttpServletRequest request, Integer fileDuration, Long fileSize) { diff --git a/airsonic-main/src/main/java/org/airsonic/player/service/TranscodingService.java b/airsonic-main/src/main/java/org/airsonic/player/service/TranscodingService.java index a98ca944..9c2a143a 100644 --- a/airsonic-main/src/main/java/org/airsonic/player/service/TranscodingService.java +++ b/airsonic-main/src/main/java/org/airsonic/player/service/TranscodingService.java @@ -216,6 +216,7 @@ public class TranscodingService { } parameters.setMaxBitRate(maxBitRate); + parameters.setExpectedLength(getExpectedLength(parameters)); parameters.setRangeAllowed(isRangeAllowed(parameters)); return parameters; } @@ -488,6 +489,32 @@ public class TranscodingService { return matches != null && matches.length > 0; } + /** + * Returns the length (or predicted/expected length) of a (possibly padded) media stream + */ + private Long getExpectedLength(Parameters parameters) { + MediaFile file = parameters.getMediaFile(); + + if (!parameters.isDownsample() && !parameters.isTranscode()) { + return file.getFileSize(); + } + Integer duration = file.getDurationSeconds(); + Integer maxBitRate = parameters.getMaxBitRate(); + + if (duration == null) { + LOG.warn("Unknown duration for " + file + ". Unable to estimate transcoded size."); + return null; + } + + if (maxBitRate == null) { + LOG.error("Unknown bit rate for " + file + ". Unable to estimate transcoded size."); + return null; + } + + // Over-estimate size a bit (2 seconds) so don't cut off early in case of small calculation differences + return (duration + 2) * (long)maxBitRate * 1000L / 8L; + } + private boolean isRangeAllowed(Parameters parameters) { Transcoding transcoding = parameters.getTranscoding(); List steps = Arrays.asList(); @@ -498,7 +525,12 @@ public class TranscodingService { steps = Arrays.asList(settingsService.getDownsamplingCommand()); } else { - return true; // neither transcoding or downsampling + return true; // neither transcoding nor downsampling + } + + // Verify that were able to predict the length + if (parameters.getExpectedLength() == null) { + return false; } // Check if last configured step uses the bitrate, if so, range should be pretty safe @@ -540,6 +572,7 @@ public class TranscodingService { public static class Parameters { private boolean downsample; + private Long expectedLength; private boolean rangeAllowed; private final MediaFile mediaFile; private final VideoTranscodingSettings videoTranscodingSettings; @@ -575,6 +608,14 @@ public class TranscodingService { this.rangeAllowed = rangeAllowed; } + public Long getExpectedLength() { + return this.expectedLength; + } + + public void setExpectedLength(Long expectedLength) { + this.expectedLength = expectedLength; + } + public void setTranscoding(Transcoding transcoding) { this.transcoding = transcoding; } From a6cdba4a4457b8a976c93baebffd49baf14170b4 Mon Sep 17 00:00:00 2001 From: Evan Harris Date: Wed, 19 Jun 2019 21:46:03 -0500 Subject: [PATCH 4/6] Added a warning if stream output exceeds predicted size --- .../org/airsonic/player/controller/StreamController.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/airsonic-main/src/main/java/org/airsonic/player/controller/StreamController.java b/airsonic-main/src/main/java/org/airsonic/player/controller/StreamController.java index 29dbb31c..e5ae6dee 100644 --- a/airsonic-main/src/main/java/org/airsonic/player/controller/StreamController.java +++ b/airsonic-main/src/main/java/org/airsonic/player/controller/StreamController.java @@ -255,6 +255,12 @@ public class StreamController { sendDummyDelayed(buf, out); } } else { + if (fileLengthExpected != null && bytesWritten <= fileLengthExpected + && bytesWritten + n > fileLengthExpected) { + LOG.warn("Stream output exceeded expected length of {}. It is likely that " + + "the transcoder is not adhering to the bitrate limit or the media " + + "source is corrupted or has grown larger", fileLengthExpected); + } out.write(buf, 0, n); bytesWritten += n; } From 1e219673033f342e4eb51184d534f655947c5ef5 Mon Sep 17 00:00:00 2001 From: Evan Harris Date: Mon, 16 Dec 2019 15:51:18 -0600 Subject: [PATCH 5/6] Fix checkstyle issues for merge --- .../org/airsonic/player/service/TranscodingService.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/airsonic-main/src/main/java/org/airsonic/player/service/TranscodingService.java b/airsonic-main/src/main/java/org/airsonic/player/service/TranscodingService.java index 9c2a143a..1037d5da 100644 --- a/airsonic-main/src/main/java/org/airsonic/player/service/TranscodingService.java +++ b/airsonic-main/src/main/java/org/airsonic/player/service/TranscodingService.java @@ -520,11 +520,9 @@ public class TranscodingService { List steps = Arrays.asList(); if (transcoding != null) { steps = Arrays.asList(transcoding.getStep3(), transcoding.getStep2(), transcoding.getStep1()); - } - else if (parameters.isDownsample()) { + } else if (parameters.isDownsample()) { steps = Arrays.asList(settingsService.getDownsamplingCommand()); - } - else { + } else { return true; // neither transcoding nor downsampling } From 72e758bd3fe1934be5bae8519300080e48419391 Mon Sep 17 00:00:00 2001 From: Evan Harris Date: Mon, 16 Dec 2019 21:01:56 -0600 Subject: [PATCH 6/6] Fixed integration tests to accomodate transcoding stream padding Renamed the transcoding stream tests to have more specific names. --- .../src/test/resources/blobs/stream/dance/responses/1.dat | 4 ++-- .../src/test/resources/blobs/stream/dead/responses/1.dat | 4 ++-- .../api/{stream-flac.feature => stream-flac-as-mp3.feature} | 2 +- .../api/{stream-m4a-vbr.feature => stream-m4a-as-mp3.feature} | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) rename integration-test/src/test/resources/features/api/{stream-flac.feature => stream-flac-as-mp3.feature} (87%) rename integration-test/src/test/resources/features/api/{stream-m4a-vbr.feature => stream-m4a-as-mp3.feature} (87%) diff --git a/integration-test/src/test/resources/blobs/stream/dance/responses/1.dat b/integration-test/src/test/resources/blobs/stream/dance/responses/1.dat index 6a89b6bf..2a31737d 100644 --- a/integration-test/src/test/resources/blobs/stream/dance/responses/1.dat +++ b/integration-test/src/test/resources/blobs/stream/dance/responses/1.dat @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:d197053e46e2ddf49f0230885d6fbc224a9d634be6462d965be256729e7fbdd2 -size 10238118 +oid sha256:608519910657cb59f64d904605794f9df97bf92a725d420f002548f825ec4524 +size 10272000 diff --git a/integration-test/src/test/resources/blobs/stream/dead/responses/1.dat b/integration-test/src/test/resources/blobs/stream/dead/responses/1.dat index 48eafcf2..be66a977 100644 --- a/integration-test/src/test/resources/blobs/stream/dead/responses/1.dat +++ b/integration-test/src/test/resources/blobs/stream/dead/responses/1.dat @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:685a5981d4098ff310c6a9b15587cbcb3878194014699d7f9b60013385ef9aed -size 5951995 +oid sha256:6e67621051eba3527bed4d5a8b30cf676089837feaf1ce6919ce84068feb5789 +size 6000000 diff --git a/integration-test/src/test/resources/features/api/stream-flac.feature b/integration-test/src/test/resources/features/api/stream-flac-as-mp3.feature similarity index 87% rename from integration-test/src/test/resources/features/api/stream-flac.feature rename to integration-test/src/test/resources/features/api/stream-flac-as-mp3.feature index 1045915b..6b690c95 100644 --- a/integration-test/src/test/resources/features/api/stream-flac.feature +++ b/integration-test/src/test/resources/features/api/stream-flac-as-mp3.feature @@ -9,5 +9,5 @@ Feature: Stream API for FLAC When A stream is consumed Then Print debug output Then The response bytes are equal - Then The length headers are absent + Then The length headers are correct diff --git a/integration-test/src/test/resources/features/api/stream-m4a-vbr.feature b/integration-test/src/test/resources/features/api/stream-m4a-as-mp3.feature similarity index 87% rename from integration-test/src/test/resources/features/api/stream-m4a-vbr.feature rename to integration-test/src/test/resources/features/api/stream-m4a-as-mp3.feature index 2c863ad0..5b5149f2 100644 --- a/integration-test/src/test/resources/features/api/stream-m4a-vbr.feature +++ b/integration-test/src/test/resources/features/api/stream-m4a-as-mp3.feature @@ -9,5 +9,5 @@ Feature: Stream API for VBR M4A When A stream is consumed Then Print debug output Then The response bytes are equal - Then The length headers are absent + Then The length headers are correct