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

Update main.c with new help text #708

Merged
merged 5 commits into from
Jun 26, 2024
Merged

Update main.c with new help text #708

merged 5 commits into from
Jun 26, 2024

Conversation

H2Swine
Copy link
Contributor

@H2Swine H2Swine commented Jun 11, 2024

I updated help text from void show_help(void) definition, to new line 1397
Most are reorderings by topic, some minor rewording and reformatting.

Possibly one needs to take a stand on when one should use double quotation marks. Is that shell-independent? Anyway I did use double quotation marks in the explanation of -6/-7/-8 because it was already used under -A. But also, what about in --output-name and --output-prefix?

From the top:
General options, ordered: -v, -h, -H and then the operations, then what to do about the output file, then user feedback (-s and -w)
Encoding options: Reordering; there is no need to have --lax etc. first, and ordinary users would probably not read tagging commands first. Also, they fit on the same indentation

  • Headline says what is default, then -V and compression levels .
  • Put quotation marks around the apod functions in -6/-7/-8 because that is under -A (see above) .
  • Then ordered the switches as they appear in the preset synonyms (those that use -m/-M at least). Lightly reworded.
  • Under -A a typo fixed and plural "function(s)".
  • Reordering -e after the models, -p after -q . then --lax, --limit, and then -j and --ignore-chunk-sizes (not sure where to put the latter) .
  • ... and only then everything about tags, seekpoints, padding

Format options reordered and slightly reworded:

  • I think --ogg is to be considered a format option. I put it first. I think I got the --serial-number explanation correct, not completely sure.
  • Put --force-raw-format last to link with the raw options
  • raw options (no capital r, it is a sub-headline) says what is mandatory, ordered with those mandatory for both encoding and decoding first.

Negative options: specified that latter takes precedence. Ordered them according to alphabet.

I updaded from void show_help(void) definition, line 1281 to new line 1397
Most are reorderings by topic, some minor rewording and reformatting.

Possibly one needs to take a stand on when one should use double quotation marks.  Is that shell-independent?  Anyway I did use double quotation marks in the explanation of -6/-7/-8 because it was already used under -A.  
But also, what about in --output-name and --output-prefix? 

From the top: 
* General options, ordered: -v, -h, -H and then the operations, then what to do about the output file, then user feedback (-s and -w)
* Encoding options: Reordering; there is no need to have --lax etc. first, and ordinary users would probably not read tagging commands first.  Also, they fit on the same indentation 
 . Headline says what is default, then -V and compression levels
 . Put quotation marks around the apod functions in -6/-7/-8 because that is under -A (see above)
 . Then ordered the switches as they appear under the presets (those that use -m/-M at least).  Lightly reworded.  Under -A a typo fixed and plural "function(s)". 
 . -e after the models, -p after -q
 . then --lax, --limit, and then -j and --ignore-chunk-sizes (not sure where to put the latter)
 . ... and only then everything about tags, seekpoints, padding
* Format options reordered and slightly reworded: 
 . --ogg is a format option I think. I put it first. I think I got the --serial-number explanation correct, not completely sure.
 . --force-raw-format last to link with the raw options
 . raw options (no capital r, it is a sub-headline) says what is mandatory
* Negative options: specified that latter takes precedence. Ordered them according to alphabet.
Copy link
Collaborator

@ktmf01 ktmf01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution! I think this is an improvement, but I have a few remarks I'd like to discuss.

src/flac/main.c Outdated Show resolved Hide resolved
src/flac/main.c Outdated Show resolved Hide resolved
printf(" --sample-rate=# Sample rate in Hz in raw input\n");
printf(" --input-size=# Size of the raw input in bytes\n");
printf("\n");
printf("Negative options (rightmost applied takes precedence):\n");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is best placed here, because it applies to all options, not just negative ones. AFAIK flac -8l32 is different from flac -l32 -8. Maybe add a note at the top, before general options, something like: `the order in which options are applied is important, options applied last take precedence over options applied first.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is maybe a new issue, combining -d, -t and -a don't work that way. And now -t makes checks -d don't.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, interesting. I usually don't like changing behaviour because it breaks scripts, but this seems wrong. If I use -t and -a together I get a warning that that combination is not possible. Maybe I should add the same for any combination of -d, -t and -a?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is turning into a new issue especially with the latter paragraph here, but to pursue the train of thought:

If anyone has used -t and -d together - say, mistakenly thinking that -dt is the test-decode option - then erring out on that might break scripts too. As for other combinations I see that -da and -ad both work as -a. And -t -c (which isn't too sensible?!) works as -t. One may change it to "last one decides", but let me suggest two or three other ways out:

  • One is to -td or -ta work as -d or -a would, but invoke the extra tests of -t. Change is biggest for -td which as of now doesn't output any file. It can be "rationalized" by saying that "-t switches to decode with all tests, but switches off output; -d / -a will switch on output but not switch off the tests". With that logic, -dt should work as currently. Then -at will either have to change (but currently it errs out, so how many use it?) or stick with some inconsistency.
  • The other is to let -d and -a actually do those extra tests that -t does, but only report warning. Unexpected to those who invoke -w?
  • Probably not a good idea, but still thinking aloud: had you not already made -t more zealous (reverting that is maybe not a good idea at this stage), you could have considered the -V switch. Provided that I understand correctly that -V does nothing except when encoding? There is no natural analog to decoding-the-encode, but had one started from scratch, -tV or -dV or -aV could have been the extra zealous one that goes to greater lengths to catch all errors. But again, with the recent changes to -t, it might just be dumb to do that?

Since this brings up how much checks -d and -a should do, it is maybe natural to ask for the demand for a "fast test" option that doesn't decode. As you have pointed out elsewhere, FLAC is maybe the codec least in need for this since it decodes so fast, but still even OptimFROG has a verification that is faster than FLAC's. I was initially thinking that one could implement some -t --no-md5-sum for non-decoding verification (because what do you need the audio for if not to MD5 it?), but if I am right, -t checks more about the audio than the MD5? (Like, decoded values don't exceed -2^(N-1) ... 2^(N-1)-1?)
Anyway I think that if you consider to change what is tested, then a fast-verification option could also be part of the consideration.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If anyone has used -t and -d together - say, mistakenly thinking that -dt is the test-decode option - then erring out on that might break scripts too.

Yes, but I think breaking scripts isn't a bad thing here.

src/flac/main.c Outdated Show resolved Hide resolved
src/flac/main.c Outdated Show resolved Hide resolved
@ktmf01 ktmf01 merged commit 9dd697b into xiph:master Jun 26, 2024
14 checks passed
@H2Swine
Copy link
Contributor Author

H2Swine commented Nov 10, 2024

Maybe not the right place to comment, I don't know (edit: @ktmf01 , at least you see it?) - but one piece of information is wrong (and, my fault). What is now line 1413, says

printf("raw format options: (all options mandatory for encoding from raw input,\n");

But --input-size is only to be used when encoding from stdin.

@ktmf01
Copy link
Collaborator

ktmf01 commented Nov 11, 2024

Commenting on a closed issue or PR is usually quite prone to being forgotten, even if I get a notification.

@H2Swine H2Swine deleted the patch-1 branch December 18, 2024 15:28
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.

2 participants