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.

wantehchang · 2025-01-17T23:31:44Z

tests/test_cmd_stdin.sh

+  "${AVIFENC}" --stdin "${ENCODED_FILE_STDIN}" "${ENCODED_FILE_STDIN}" && exit 1
+  "${AVIFENC}" "${ENCODED_FILE_STDIN}" --stdin "${ENCODED_FILE_STDIN}" && exit 1
+  "${AVIFENC}" "${ENCODED_FILE_STDIN}" "${ENCODED_FILE_STDIN}" --stdin && exit 1
+


There is trailing whitespace in this line.

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.

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.

wantehchang · 2025-01-19T18:39:48Z

apps/avifenc.c

+        return AVIF_FALSE;
+    }
+    if (input->files[input->fileIndex].filename == AVIF_FILENAME_STDIN) {
+        return !feof(stdin); // stdin


Nit: The "// stdin" comment could be omitted because it is obvious from the context (AVIF_FILENAME_STDIN and !feof(stdin)).

wantehchang · 2025-01-19T18:40:51Z

apps/avifenc.c

@@ -651,7 +653,7 @@ static char * avifStrdup(const char * str)
    return dup;
 }

-static avifBool avifCodecSpecificOptionsAdd(avifCodecSpecificOptions * options, const char * keyValue)
+static avifBool avifCodecSpecificOptionsAddKeyValue(avifCodecSpecificOptions * options, const char * key, size_t keyLength, const char * value)


Nit: Document that key is not null-terminated and value is null-terminated. OK if you think this is obvious.

wantehchang · 2025-01-19T18:41:19Z

apps/avifenc.c

    success = AVIF_TRUE;
 cleanup:
    ++options->count;
    free(oldKeys);
    free(oldValues);
    return success;
 }
+static avifBool avifCodecSpecificOptionsAdd(avifCodecSpecificOptions * options, const char * keyValue)


Nit: Add a blank line before this line.

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

apps/avifenc.c

+        const char * key = src->codecSpecificOptions.keys[i];
+        const char * value = src->codecSpecificOptions.values[i];
+        if (!avifCodecSpecificOptionsAddKeyValue(&dst->codecSpecificOptions, key, strlen(key), value)) {
+            return AVIF_FALSE;


Optional: Should we print an error message here? See the fprintf statement at line 885:

for (int i = 0; i < settings->codecSpecificOptions.count; ++i) { if (avifEncoderSetCodecSpecificOption(encoder, settings->codecSpecificOptions.keys[i], settings->codecSpecificOptions.values[i]) != AVIF_RESULT_OK) { fprintf(stderr, "ERROR: Failed to set codec specific option: %s = %s\n", settings->codecSpecificOptions.keys[i], settings->codecSpecificOptions.values[i]); return AVIF_FALSE; } }

wantehchang · 2025-01-19T18:44:55Z

apps/avifenc.c

+            input.files[input.filesCount].duration = settings.outputTiming.duration;
+            input.files[input.filesCount].settings = pendingSettings;
+            memset(&pendingSettings, 0, sizeof(pendingSettings));
+            ++input.filesCount;


Optional: These five lines (1511-1515) are the same as lines 1966-1970 except for the filename:

} else { // Positional argument input.files[input.filesCount].filename = arg; input.files[input.filesCount].duration = settings.outputTiming.duration; input.files[input.filesCount].settings = pendingSettings; memset(&pendingSettings, 0, sizeof(pendingSettings)); ++input.filesCount; }

Create a new function for these five lines, with the filename as an input parameter.

wantehchang · 2025-01-19T18:46:11Z

apps/avifenc.c

+                // there must be an output file specified with or without --output.
+                // Invalid patterns will be rejected later on.
+
+                // Merge all the settings seen so far and associate them to stdin only.


Nit: Change "to" to "with"

wantehchang · 2025-01-19T18:47:16Z

tests/test_cmd.sh

-    rm -f -- "${ENCODED_FILE}" "${ENCODED_FILE_WITH_DASH}" "${DECODED_FILE}" "${OUT_MSG}"
+    rm -f -- "${ENCODED_FILE}" "${ENCODED_UTF8_FILE}" "${ENCODED_FILE_REFERENCE}" \
+             "${ENCODED_FILE_WITH_DASH}" "${DECODED_FILE}" "${DECODED_UTF8_FILE}" \
+             "${OUT_MSG}"


Optional: Move this unrelated bug fix to its own pull request.

wantehchang · 2025-01-19T18:49:19Z

tests/test_cmd_stdin.sh

+  "${AVIFENC}" --stdin && exit 1
+  "${AVIFENC}" --stdin "${ENCODED_FILE_STDIN}" "${ENCODED_FILE_STDIN}" && exit 1
+  "${AVIFENC}" "${ENCODED_FILE_STDIN}" --stdin "${ENCODED_FILE_STDIN}" && exit 1
+  "${AVIFENC}" "${ENCODED_FILE_STDIN}" "${ENCODED_FILE_STDIN}" --stdin && exit 1


Nit: In these three lines (96-98), use different input and output filenames to make the tests more realistic.

I.e., replace the first "${ENCODED_FILE_STDIN}" with "${INPUT_Y4M}".

Note: The input file should exist so that these three avifenc command lines succeed if --stdin is not specified:

"${AVIFENC}" "${INPUT_Y4M}" "${ENCODED_FILE_STDIN}"

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

wantehchang reviewed Jan 18, 2025

View reviewed changes

wantehchang approved these changes Jan 19, 2025

View reviewed changes

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 Jan 17, 2025

wantehchang commented Jan 17, 2025

wantehchang Jan 18, 2025

wantehchang Jan 19, 2025

tongyuantongyu commented Jan 19, 2025

wantehchang left a comment

wantehchang Jan 19, 2025

wantehchang Jan 19, 2025

wantehchang Jan 19, 2025

wantehchang Jan 19, 2025

wantehchang Jan 19, 2025

wantehchang Jan 19, 2025

wantehchang Jan 19, 2025

wantehchang Jan 19, 2025

wantehchang Jan 19, 2025

wantehchang Jan 19, 2025

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

Are you sure you want to change the base?

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

Conversation

y-guyon commented Jan 17, 2025

Choose a reason for hiding this comment

wantehchang commented Jan 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tongyuantongyu commented Jan 19, 2025

wantehchang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment