Skip to content
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.

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

"--explain" does not state what can be maximal allowed value for "--max-lpc-order" #699

Open
Rollinnn opened this issue May 2, 2024 · 12 comments

Comments

@Rollinnn
Copy link

Rollinnn commented May 2, 2024

It only states that: "Max LPC order; 0 => only fixed predictors. Must be <= 12 for Subset streams if sampleate is <=48kHz"
No mention of maximal value 32.

Maximal value for --qlp-coeff-precision is not stated too.

Also description for --rice-partition-order seems to be a bit incorrect: "<...> the default is -r 0 <...>".
But "the default" is -r 5 actually.

FLAC 1.4.3

@H2Swine
Copy link

H2Swine commented May 4, 2024

(edit: messed up tabs in attachment. New uploaded)

Yeah ... the explain text has not been the top priority, and options that were long gone since at least 1.1.4 were not removed from the text until 1.4.0. Still it refers to https://xiph.org/flac/documentation_bugs.html which is obsolete for more than seven years.
The order is a mess too.

More broadly, one could discuss - or just make an executive decision on:

  • Should SUBSET bounds be stated? (I think yes - it is missing for -r. And that also goes for absolute bounds where subset does not make further restrictions, like, it is not mentioned that block size must be at least 16.)
  • Should bounds beyond subset be stated? (I am fine with either, but maybe stay consistent?)
  • Should it mention when options are "ignored if nonsense" (like, -m/-M for non-stereo? About "-p" it says "does nothing" when -l 0, one could choose between "ignored" and "does nothing" if one wants consistent wording ...)
  • ... and precedence. Rightmost-takes-precedence, but with some exceptions ... -M0 = -0M = -1, but -dtdtd = -t because -t switches off output ... ?
  • How far to go to explain advanced usage? (I think that when the explanation for -A states that more than one function can be specified, it could also say "semicolon-separated", but I am fine with not explaining parameters.)
  • How far to go to recommend? (Surely it should stop saying that -r above 4 does not usually help much, when default is 5. I'd go out on a limb and say that one could well recommend -b to be a multiple of ... heck, due to -0 it couldn't say more than 128. And maybe for some unknown reasons, someone would want 576 = 9*64. But multiples of 64 means it allows a decent partition order.)
  • Language: Should one capitalize more words like SUBSET and CONSTANT? (It does for e.g. PADDING.)

I took the liberty to re-draft the whole thing. Re-ordered with General options, then Encoding options, etc. Hopefully got most things factually correct. Because I would surely make a mess out of things by trying to make a pull request, I am simply attaching it here. In "the right margin", to the right of the 80th character, I put some remarks preceeded by %%%.

Comments, anyone?

flacexplain-suggestion.txt

@ktmf01
Copy link
Collaborator

ktmf01 commented May 16, 2024

Thanks for your suggestions @H2Swine, it indeed seems long overdue.

Could I perhaps persuade you to try to make a PR out of this? Makes comparing and commenting a whole lot easier. I'll walk you through it.

  1. Go to https://github.com/xiph/flac and use the 'fork' button in the top right, just below the search button.
  2. Go to your new fork (which will probably be at https://github.com/H2Swine/flac unless you renamed it) and go to "branches". Or click the dropdown with branches and select View All branches, which will do the same. You'll end up in https://github.com/H2Swine/flac/branches
  3. Create a new branch. You should probably name it 'improve-explain' or something. It is common on github to create a seperate branch for each PR you file.
  4. In your new branch, go to src/flac/main.c and click the edit button. That one is pretty well hidden, it is a pencil icon on the right, just above the first line
  5. Scroll down to line 1400-something and edit the explain text. Yes, you will be editing C-source, but it is nothing fancy and the pattern is easy to recognize
  6. When done, click the 'commit changes' button. You can do all changes in one commit or one commit per change, whatever you like. Enter a nice description for the change, select that you'll be committing to the new branch. (Step 3 could have been skipped when selecting a new branch here)
  7. Go back to the main page for the new branch (probably https://github.com/H2Swine/flac/tree/improve-explain) and use the contribute button to make a new PR. Once more add some explain text (maybe reference this issue by adding the text #699) and we have a PR in which I can make some annotations per item.

If you could do that, that would be great! As a bonus, you'll get properly credited of course. Note: depending on your github privacy settings, a 'github noreply' address will be set as the commit email. Maybe you'll want to review that first.

@H2Swine
Copy link

H2Swine commented May 21, 2024

Didn't look too intimidating, let's see if I've made it as far as to step 6? H2Swine@bbc6e7f

Input requested on what is not included in current --explain text, that should be? EDIT: I could have updated myself, -j is already in. Those commented as undocumented should stay so, including --channel-map=none which is actually sometimes even needed?

I see that the --help file could use some help too, but ... old dogswine and new trick, one at a time.

@ktmf01
Copy link
Collaborator

ktmf01 commented May 22, 2024

Yes, that looks like what I had in mind. Note that after you've created a PR (step 7), you can still add more commits to that branch. So, creating a PR is not the final step per se.

Considering --channel-map, I'm not what use it has these days. The hidden use of --channel-map=none is gone with #507 I think. Maybe it can be useful to encode a WAV which has no channel mask. Also, the name of the option is wrong: it doesn't map any channels, it assigns a mask to them AFAIK. Then again, I am not really sure. Maybe the option should be kept hidden?

Some more comments (not all):

  • Language: Should one capitalize more words like SUBSET and CONSTANT? (It does for e.g. PADDING.)

I don't know, I don't like allcaps in general. The old FLAC spec used to be full of ALLCAPS but I've removed them all. It is probably useful in a terminal text without any other form of emphasis, but I think FIXED and SUBSET are words not usually needing emphasis. I think placeholders like INPUTFILE are good to have ALLCAPS, and names, extensions and abbreviations like WAVE, FLAC and PCM.

  • Should SUBSET bounds be stated? (I think yes - it is missing for -r. And that also goes for absolute bounds where subset does not make further restrictions, like, it is not mentioned that block size must be at least 16.)

I'm not sure. Currently we have 4 levels of help text:

  • the "short help" when flac is invoked with no options
  • the "help" for flac -h
  • the "explain" for flac -H
  • the manpage

For each boundaries have to be set. Now that I think about it, maybe -H should be removed altogether in favor of referring to the manpage

  • Should bounds beyond subset be stated? (I am fine with either, but maybe stay consistent?)

Same issue: the manpage is meant to be exhaustive, the common help is meant to document most options succinctly... What is the explain option meant to do?

@ktmf01
Copy link
Collaborator

ktmf01 commented May 22, 2024

Maybe the explain text should be removed and the -H option should display the file https://github.com/xiph/flac/tree/master/man/flac.md?plain=1 verbatim? Maybe with a few tweaks like removing emphasis or backslashes? That will add quite a bit of weight to the binary though.

@H2Swine
Copy link

H2Swine commented May 22, 2024

Yeah, good idea consider first whether it makes sense to have that explain text at all.

If the executable is to be kept slim, could the "man page" (whether it will stay that way) be distributed as a separate file?

@ktmf01
Copy link
Collaborator

ktmf01 commented May 23, 2024

If the executable is to be kept slim, could the "man page" (whether it will stay that way) be distributed as a separate file?

That is already the case. See flac-1.4.3-win.zip, it has a 'manuals' directory with them in markdown format.

On unix systems, the manpage is in the ancient groff format, which can be read by the man utility. Just invoking man flac or man metaflac gets you the help. Currently the man pages are generated from the much easier to use markdown files with pandoc.

It would be most helpful if on Windows they could be compiled into a .chm file, but Pandoc doesn't support this. Maybe the second best thing is to generate an .html help file to go along the binaries? That is probably easier to read (and less intimidating to novice users) than a markdown file.

@H2Swine
Copy link

H2Swine commented May 26, 2024

For the novice user who actually needs it, .html is better than .md. But I wonder if the .md is just fine if you replace away the escape slashes and the formatting - is it feasible in order to maintain just one? And the EXAMPLES could get an indent if that solves anything.

But what is then the sane workflow? Have a look at the --help text and work with the .md files for it to replace the --explain text?

(Also I think there is a double asterisk missing in flac.md :

The option \--apply-replaygain-which-is-not-lossless\[=\<specification\>\]**
)

@ktmf01
Copy link
Collaborator

ktmf01 commented May 26, 2024

The problem with removing the backslashes is that it renders wrong when someone views it with an actual markdown editor or viewer. Also, as we already use pandoc to convert to groff, converting to html isn't too much work to automate.

@H2Swine
Copy link

H2Swine commented May 26, 2024

I didn't think of removing it from the document. I thought, if you want a plaintext output then you could code it as "fetch md file, query replace, view it to user. But whatever is most convenient ...

... which is: just don't work with the --explain text until further notice?

@H2Swine
Copy link

H2Swine commented May 29, 2024

Anyway, if I am to look into the --help text instead of the --explain text, I'd anyway like to get the facts right. From common sense and text-searching main.c I have the following hunches:

  • --no-utf8-convert is found under general options, but is really an encoding-only option?
  • --ogg and --serial-number are format options for encoding-only? (And the latter requires --ogg to be set?)
  • Format options (except previous item!) are decoding only except for raw, where decoding requires --endian and --sign (no others!) and encoding requires all?
  • --ignore-chunk-sizes works with RF64 too it looks, does it also apply to W64 - IOW, all uncompressed inputs but raw?

I'm ignoring the "Be sure to read the list of known bugs at:"

@ktmf01
Copy link
Collaborator

ktmf01 commented May 29, 2024

I don't know the answers yet, I'll have to find some time to check the code. The manpage says the following

--serial-number=#
When used with --ogg, specifies the serial number to use for the first Ogg FLAC stream, which is then incremented for each additional stream. When encoding and no serial number is given, flac uses a random number for the first stream, then increments it for each additional stream. When decoding and no number is given, flac uses the serial number of the first page.

But I'll have to check of course

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

No branches or pull requests

3 participants