Skip to content

Allow --skip and --until with a total sample count of 0#838

Merged
ktmf01 merged 3 commits intoxiph:masterfrom
Stefan-Olt:master
Aug 20, 2025
Merged

Allow --skip and --until with a total sample count of 0#838
ktmf01 merged 3 commits intoxiph:masterfrom
Stefan-Olt:master

Conversation

@Stefan-Olt
Copy link
Contributor

This patch will allow use of --skip and --until when the total sample count is 0.

--until is not possible if relative offset from the end (error is shown). Relative from --skip value is possible.
--skip with a value after the end of the file will result in a seek error. As there is no information why seeking has failed a more detailed message is not possible.

@H2Swine
Copy link
Contributor

H2Swine commented Jul 1, 2025

To be clear, will flac --until 0 infile process nothing or will it process everything?

@Stefan-Olt
Copy link
Contributor Author

Using --until 0 will process nothing and bring an error message: ERROR, --until value is before --skip point This message is somewhat confusing, because:

  • The user did not use --skip (if you don't use --skip the skip value is 0)
  • The value is not before the skip point, but at the point.

It would probably be better would to split it into two error messages and make an individual one for --until 0. Note that --until -0 is treated as relative with 0 samples to end of stream, effectively processing everything.

My patch did not affect this behavior at all. If you mean encoding, I don't know, because my patch only affects decoding.

@H2Swine
Copy link
Contributor

H2Swine commented Jul 2, 2025

My question was on what it was supposed to, apologies for being unclear. In part asking because the help text says --until stops before the number given. Which holds if you start counting at 0 like code does, but not all humans do ... Likely the help text should say that --until # processes precisely # samples - starting from beginning though from skip point if "+" and from end if "-".

Using --until 0 will process nothing and bring an error message: ERROR, --until value is before --skip point

Yes, as of now it does. Both for encoding and decoding.

My patch did not affect this behavior at all. If you mean encoding, I don't know, because my patch only affects decoding.

I would want --until to work precisely the same for encoding and for decoding. Of course error messages should make sense too.

@Stefan-Olt
Copy link
Contributor Author

Yes, it definitely makes sense to have --until work the same in encoding and decoding. All my patch did is to allow usage of --skip and --until in a specific situation during decoding (flac file does not contain total sample count in header), in which it currently fails with an error message.
I had a look at the encoder, it has a similar limitation which should affect two cases: Encoding from a pipe and encoding from a flac file that does not contain total sample count in header (like decoding). Fixing the second case should be the same as the decoder, but for implementing that from a pipe it needs to be discussed how this should be done: As we cannot seek we have to discard all data before --skip and after --until, or we terminate after --until is reached. Not sure what the best approach is. My personal opinion: Do not allow --skip and --until with piped input, because there is most likely a better way to achieve the the goal of encoding some parts of the input without having to pass over and then discard a lot of data.

H2Swine referenced this pull request Jul 3, 2025
Test to comment each change
@harrypm
Copy link

harrypm commented Jul 24, 2025

This has very high value implications for FM RF Archival usage as flac is both a capture standard (since v1.5.0 a defacto) and a gen/cold storage standard for archives, these are files that are upto 11 hours long at 40-65MSPS 8-12bit rates.

@ktmf01
Copy link
Collaborator

ktmf01 commented Aug 14, 2025

Just as a heads-up: the reason I haven't merged this, is because CIFuzz doesn't run correctly. I'm currently trying to fix this. I'll rerun tests when this is finished

Just to trigger checks
@ktmf01
Copy link
Collaborator

ktmf01 commented Aug 20, 2025

@Stefan-Olt could you synchronize you branch (with the button pictured below, at https://github.com/Stefan-Olt/flac/tree/master), and then check whether it still works as you intended?

afbeelding

@Stefan-Olt
Copy link
Contributor Author

I synchronized my branch and it works fine just like before. 15 of the 18 automatic tests have already passed, CIFuzz is still in progress.

@ktmf01
Copy link
Collaborator

ktmf01 commented Aug 20, 2025

I didn't mean the automated checks. Just to be sure: did you check whether your PR still does what it intends to do? Can you still use skip and until with a total sample count of 0? Because no automated tests for that behaviour have been added yet.

@Stefan-Olt
Copy link
Contributor Author

Stefan-Olt commented Aug 20, 2025

I didn't mean the automated checks. Just to be sure: did you check whether your PR still does what it intends to do? Can you still use skip and until with a total sample count of 0? Because no automated tests for that behaviour have been added yet.

Sorry for being unclear: In the first sentence I was referring to manual testing, works just fine like before (including problem cases like skip or until after end of file). Only the automated tests are not completed yet.

@ktmf01 ktmf01 merged commit 9547dbc into xiph:master Aug 20, 2025
18 checks passed
@ktmf01
Copy link
Collaborator

ktmf01 commented Aug 20, 2025

Merged, thanks!

Tremus pushed a commit to Tremus/flac that referenced this pull request Sep 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants