@tari @muff1nman any opinion on this, since you were the ones discussing it in #1117

As I said there, because this estimate can still be wrong I'm not comfortable making it the only option. Reintroducing the "allow seeking" user-facing option and changing transcode-with-seek behavior (when enabled by the user) to do this seems okay, though I'm not sure how player in general would handle having garbage inserted at the end of a file.

I'm kind of holding off on making more changes to this until #1124 gets merged, because they touch some common parts of the code. And I have further changes planned that will make this much safer even without a configuration option to enable it.

As to how players handle extra data tacked on to pad out the transfer to the proper length, I kind of disagree with the term "garbage", and would point out that the code already did that in some circumstances for streaming sources, so appears to be at least somewhat of a time-tested mechanism.

I should also point out that the change that broke seeking for transcodes is a very recent change that probably hasn't seen that much exposure to the user community at large. I suspect that there might be a decent amount of fallout from the loss of that capability and its other side effects.

The existing practice of inserting garbage into streams has also made me nervous when looking at the code, but I haven't touched it because it appears to work and I haven't been interested in that use case.

I agree that people will probably miss seeking, but I know people will also miss having streams that work correctly. Not all users will care about both though, so an option seems to make sense. Ultimately I'd hope there is a more correct and integrated solution, but the existing change was done this way because it's not possible to fix known problems to a high degree of reliability without making this tradeoff.

@tari will you check if my isRangeAllowed() commit is acceptable to you as far as attempting to ensure safety when allowing seeking in transcodes, without the need for a user configurable option?

That's an interesting heuristic. I'm still not really happy with this solution, but it seems okay enough to work around what is mostly an unfortunate set of assumptions in the original API design that we're going to be stuck with (since we don't control all the clients). Maybe eventually we can adopt DASH as a backwards-incompatible streaming solution, and support the existing API by spooling transcoded files in their entirety to temporary storage so we can always know the exact size- trading latency (presumably less important at that point) for accuracy.

The real test will be running this for a while and seeing if it does things wrong, but I'm okay with giving it a try.

@tari Thanks for being willing to try this solution. I also just added a commit to add a warning if the stream output exceeds the expected length, so we can have some indicator when that happens in case it causes problems for players.

I think this PR is ready to be merged.

@tari Also, after this gets merged I would be willing to take a stab at addressing your stream padding concerns by working on a method to append actual valid silence frames (in the appropriate format for the output stream) rather than the current method. I'm not sure how that will work when the last silence frame may be truncated at the end of the stream, but hopefully players will see a truncated frame as invalid and will ignore it.

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

Didn't Airsonic previously truncate the duration up to the second, assume CBR and discard extra bytes in the output? It was working reasonably well, on both the web player and mobile clients (DSub). Padding with zeros shouldn't be an issue either.

Another thing : I personally don't care much about this, but how does overestimating the length work with players offering gapless playback (DSub does as far as I know)?

So far so good, I've been testing this over the last few weeks and everything appears to be working well.

Have my tentative approval, I saw no issues running this on my server.

I'll try to give it a full review of the code itself, but might not be able to do that before next week, so if somebody else can review and approve you can go ahead and merge.

It's been a while now, but IIRC this seemed to work okay for me too.

Ready to merge as soon as the test failures are handled 😄

Integration tests are failing, and from my understanding, it's only due to the fact that they should be updated : they are currently checking that the sent bytes are equal to the actual ones, but as this very pull request is made against this idea… Integration tests should be updated.

I believe that a new Then response bytes are larger test should be added in /integration-test/src/test/java/org/airsonic/test/cucumber/steps/api/StreamStepDef.java.

I'd really like to merge that in the next release, so I added the 10.6.0 milestone. I'll update that if we decide to go to 11.0 instead!

I fixed the checkstyle issues and the transcoding tests by updating the compared byte sequences and adding back the check that the length headers match. I decided not to try to change the tests to allow a non-specified amount of "additional padding" bytes since that just makes things more convoluted (and probably also overly permissive) when the comparison byte sequences would most likely need to be generated by the transcoding output code path anyway.

As mentioned previously, an even better solution would be to have the range requests only be allowed by configuration for the specific players that need them. That probably should have been done as part of the original change rather than just removing the range capability for transcodes completely, but this PR at least gets us back to the expected prior behavior in a somewhat safer manner and avoids most of the undesired side-effects caused by that change. Making those improvements is left for a later PR.

Merged, thank you for the help!