Treat avifenc --stdin as regular file input arg #2574

y-guyon · 2025-01-17T09:49:45Z

Consider --stdin to be a positional argument as if it was the name of the y4m file path containing the y4m standard input contents.

Fixes #2572.

Consider --stdin to be a positional argument as if it was the name of the y4m file path containing the y4m standard input contents.

tests/test_cmd_stdin.sh

wantehchang · 2025-01-17T23:42:50Z

@tongyuantongyu Yuan: Could you also take a look at this pull request if you have time? Thanks.

wantehchang · 2025-01-18T00:01:39Z

CHANGELOG.md

@@ -57,6 +57,7 @@ The changes are relative to the previous release, unless the baseline is specifi
  avifCropRectFromCleanApertureBox() and avifCleanApertureBoxFromCropRect().
 * Ignore descriptive properties associated after transformative ones.
 * Reject non-essential transformative properties.
+* Treat avifenc --stdin as a regular positional file path argument.


I haven't read the pull request. I have a question: where is the position of this fictitious file path argument?

Here is my expectation:

If the output file path is specified without using the -o option, the fictitious input file path argument is right before the output file path argument.

If the -o option is used, the fictitious input file path argument is at the end of the command line.

Is this what you implemented?

I have now read the pull request. What you implemented is not exactly the same as what I expected in how the options with update suffix (i.e., pendingSettings) are handled. This is perfectly fine because there are several reasonable ways to handle this. Since people's expectations may be different from what is implemented, it would be good to document somewhere how we handle the options with update suffix when --stdin is specified.

It makes no sense to use :u with --stdin because no other input is allowed. So how :u behaves with --stdin does not matter, as long as nothing is silently ignored.

I find it simpler to understand and to implement if --stdin can just be replaced with /path/to/in.y4m with identical behavior. I do not think it has to be mentioned in --help because there is no other input file name allowed, so it does not really matter where --stdin is.

Happy to revisit if needed though.

I agree that it's easier to understand if --stdin can be replaced with /path/to/in.y4m with identical behavior. This simple rule is only mentioned in the changelog. It would be good to document this in the help message.

Also, this rule needs to make an exception for the avifenc out.avif --stdin case. The exception might be worth documenting.

It would be good to document this in the help message.

I disagree. It does not provide any benefit as long as "No other input is allowed": since there is an ovious single input and a single output, the order of all arguments should not matter. The same behavior as if replaced by a file path is somewhat expected (besides the exception you mentioned), and if it is not the case, it does not really matter because there is a single input and a single output.
So describing that in the help message would just bring confusion in my opinion.

tongyuantongyu · 2025-01-19T13:56:14Z

@tongyuantongyu Yuan: Could you also take a look at this pull request if you have time? Thanks.

LGTM, though I have a small question: while we are there, can we support - as an alias of --stdin?

wantehchang

LGTM. Thanks a lot for the fix.

@tongyuantongyu Yuan: Thank you for the review!

wantehchang · 2025-01-19T18:31:13Z

CHANGELOG.md

@@ -57,6 +57,7 @@ The changes are relative to the previous release, unless the baseline is specifi
  avifCropRectFromCleanApertureBox() and avifCleanApertureBoxFromCropRect().
 * Ignore descriptive properties associated after transformative ones.
 * Reject non-essential transformative properties.
+* Treat avifenc --stdin as a regular positional file path argument.


I have now read the pull request. What you implemented is not exactly the same as what I expected in how the options with update suffix (i.e., pendingSettings) are handled. This is perfectly fine because there are several reasonable ways to handle this. Since people's expectations may be different from what is implemented, it would be good to document somewhere how we handle the options with update suffix when --stdin is specified.

wantehchang · 2025-01-19T18:38:37Z

apps/avifenc.c

@@ -215,7 +222,7 @@ static void syntaxLong(void)
    printf("                                        For JPEG, auto honors the JPEG's internal format, if possible. For grayscale PNG, auto defaults to 400. For all other cases, auto defaults to 444\n");
    printf("    -p,--premultiply                  : Premultiply color by the alpha channel and signal this in the AVIF\n");
    printf("    --sharpyuv                        : Use sharp RGB to YUV420 conversion (if supported). Ignored for y4m or if output is not 420.\n");
-    printf("    --stdin                           : Read y4m frames from stdin instead of files; no input filenames allowed, must set before offering output filename\n");
+    printf("    --stdin                           : Read y4m frames from stdin instead of files; no input filenames allowed\n");


Just wondering: why is "must set before offering output filename" removed?

The new code allows --stdin to be set after the output filename, so "must set before offering output filename" is incorrect for the new code. Does the original code also allow --stdin to be set after the output filename?

Note: When --stdin is treated as a regular positional file path argument, it seems cleaner if we disallow output.avif ... --stdin. But if the original code allows output.avif ... --stdin, the new code will need to allow it.

Just wondering: why is "must set before offering output filename" removed?

Because it is allowed.

The new code allows --stdin to be set after the output filename, so "must set before offering output filename" is incorrect for the new code. Does the original code also allow --stdin to be set after the output filename?

I did not try it but I do not see how it could detect that.
The only place where --stdin is parsed from the command line arguments just flips a boolean:

libavif/apps/avifenc.c

Lines 1475 to 1477 in 7ae69db

} else if (!strcmp(arg, "--stdin")) {

input.useStdin = AVIF_TRUE;

} else if (!strcmp(arg, "-o") || !strcmp(arg, "--output")) {

which is not checked until all other arguments were parsed.

Note: When --stdin is treated as a regular positional file path argument, it seems cleaner if we disallow output.avif ... --stdin. But if the original code allows output.avif ... --stdin, the new code will need to allow it.

I was thinking about these cases:

avifenc --stdin -o out.avif < in.y4m # Explicit avifenc --stdin out.avif < in.y4m # Expected avifenc -o out.avif --stdin < in.y4m # Allowed avifenc out.avif --stdin < in.y4m # Why not? It keeps "--stdin" close to "< in.yam".

apps/avifenc.c

tests/test_cmd.sh

tests/test_cmd_stdin.sh

y-guyon force-pushed the stdin branch 9 times, most recently from 55e529c to 0d27263 Compare January 17, 2025 12:19

Treat avifenc --stdin as regular file input arg

95888a6

Consider --stdin to be a positional argument as if it was the name of the y4m file path containing the y4m standard input contents.

y-guyon force-pushed the stdin branch from 0d27263 to 95888a6 Compare January 17, 2025 12:43

y-guyon requested a review from wantehchang January 17, 2025 13:10

wantehchang reviewed Jan 17, 2025

View reviewed changes

tests/test_cmd_stdin.sh Outdated Show resolved Hide resolved

wantehchang reviewed Jan 18, 2025

View reviewed changes

wantehchang approved these changes Jan 19, 2025

View reviewed changes

y-guyon added 3 commits January 24, 2025 11:40

Remove trailing whitespace in test_cmd_stdin.sh

fcaaf3e

Better document --stdin in avifenc --help

2f2870f

Apply other comments

4fc3fa1

y-guyon merged commit 46a46aa into AOMediaCodec:main Jan 27, 2025
35 checks passed

y-guyon deleted the stdin branch January 27, 2025 12:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Treat avifenc --stdin as regular file input arg #2574

Treat avifenc --stdin as regular file input arg #2574

y-guyon commented Jan 17, 2025

wantehchang commented Jan 17, 2025

wantehchang Jan 18, 2025

wantehchang Jan 19, 2025

y-guyon Jan 24, 2025

wantehchang Jan 24, 2025

y-guyon Jan 27, 2025 •

edited

Loading

tongyuantongyu commented Jan 19, 2025

wantehchang left a comment

wantehchang Jan 19, 2025

wantehchang Jan 19, 2025

y-guyon Jan 24, 2025 •

edited

Loading

	} else if (!strcmp(arg, "--stdin")) {
	input.useStdin = AVIF_TRUE;
	} else if (!strcmp(arg, "-o") \|\| !strcmp(arg, "--output")) {

Treat avifenc --stdin as regular file input arg #2574

Treat avifenc --stdin as regular file input arg #2574

Conversation

y-guyon commented Jan 17, 2025

wantehchang commented Jan 17, 2025

wantehchang Jan 18, 2025

Choose a reason for hiding this comment

wantehchang Jan 19, 2025

Choose a reason for hiding this comment

y-guyon Jan 24, 2025

Choose a reason for hiding this comment

wantehchang Jan 24, 2025

Choose a reason for hiding this comment

y-guyon Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

tongyuantongyu commented Jan 19, 2025

wantehchang left a comment

Choose a reason for hiding this comment

wantehchang Jan 19, 2025

Choose a reason for hiding this comment

wantehchang Jan 19, 2025

Choose a reason for hiding this comment

y-guyon Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

y-guyon Jan 27, 2025 •

edited

Loading

y-guyon Jan 24, 2025 •

edited

Loading