xiph / flac

Free Lossless Audio Codec
https://xiph.org/flac/
GNU Free Documentation License v1.3
1.58k stars 278 forks source link

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

Open Rollinnn opened 2 months ago

Rollinnn commented 2 months ago

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 commented 2 months ago

(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:

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 commented 2 months ago

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 commented 1 month ago

Didn't look too intimidating, let's see if I've made it as far as to step 6? https://github.com/H2Swine/flac/commit/bbc6e7f05f365ed28311dbfb5f4f9e42b3b899e0

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 commented 1 month ago

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:

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 : https://github.com/xiph/flac/blob/cfe3afca9b3f27f0877203570705e072f0981b2e/man/flac.md?plain=1#L608 )

ktmf01 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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:

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

ktmf01 commented 1 month ago

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

H2Swine commented 1 month ago

I made a shot at the help text. Like https://github.com/H2Swine/flac/compare/master...H2Swine-patch-for---help-text . Should have had a better line 1364 ... N00b question: I still don't do any damage by pressing "Create pull request"?

ktmf01 commented 1 month ago

Thanks! I can't think of anything bad that will happen by creating a pull request, so go ahead 😄

ktmf01 commented 2 weeks ago

@H2Swine #715 adds html docs, currently named flac.html and metaflac.html. They are self-contained, examples here and here.

The names are to mirror the manpages, in which the name is usually the same as the executable it is documenting. Maybe for these html docs the names should be the same as on the FLAC homepage, so documentation_tools_flac.html and documentation_tools_metaflac.html

Suggestions? I think that with this change, --explain can be removed as redundant.

Rollinnn commented 2 weeks ago

Suggestions? I think that with this change, --explain can be removed as redundant.

But some 3rd parties can exclude *.htlm when redistributing flac.exe...

ktmf01 commented 2 weeks ago

I propose that this text is added to the -h help

This help text summarizes all available options, for more explanation and 
examples please consult the manual. This manual is often distributed
alongside the program as a man page or an HTML file. It can also be found
online at https://xiph.org/flac/documentation_tools_flac.html