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

Support changing config for avifenc #1450

Merged
merged 18 commits into from
Sep 5, 2023

Conversation

tongyuantongyu
Copy link
Contributor

@tongyuantongyu tongyuantongyu commented Jun 24, 2023

Add :u/:update suffix syntax for options that can be changed during encoding. :u suffix instruct the option to only apply to inputs after it.

This feature is mostly useful for progressive encoding. (Which will be implemented in following PR).

@tongyuantongyu
Copy link
Contributor Author

@wantehchang do you think it's time to have this in avifenc?

@wantehchang
Copy link
Collaborator

Yuan: Thanks for the reminder. We will review this pull request in the next two weeks.

Copy link
Collaborator

@y-guyon y-guyon left a comment

Choose a reason for hiding this comment

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

This PR includes refactoring and behavior changes. Consider splitting these into multiple PR or at least multiple commits for a more efficient review process. However to save time I still reviewed most changes in this PR.

apps/avifenc.c Outdated
@@ -135,6 +207,23 @@ static void syntaxLong(void)
printf(" --imir MODE : Add imir property (mirroring). 0=top-to-bottom, 1=left-to-right\n");
printf(" --clli MaxCLL,MaxPALL : Add clli property (content light level information).\n");
printf(" --repetition-count N or infinite : Number of times an animated image sequence will be repeated. Use 'infinite' for infinite repetitions (Default: infinite)\n");
printf(" -- : Signals the end of options. Everything after this is interpreted as file names.\n");
printf("\n");
printf(" Frame options apply only to input files appeared after the option:\n");
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a breaking change. It should be documented in CHANGES.md.

apps/avifenc.c Show resolved Hide resolved
apps/avifenc.c Outdated Show resolved Hide resolved
apps/avifenc.c Outdated Show resolved Hide resolved
apps/avifenc.c Outdated Show resolved Hide resolved
apps/avifenc.c Outdated Show resolved Hide resolved
apps/shared/y4m.h Outdated Show resolved Hide resolved
tests/test_cmd_animation.sh Show resolved Hide resolved
@@ -60,14 +61,24 @@ pushd ${TMP_DIR}
"${ARE_IMAGES_EQUAL}" "${INPUT_Y4M_0}" "${DECODED_FILE}" 0 && exit 1

# Lossless test.
"${AVIFENC}" -s 8 "${INPUT_Y4M_0}" "${INPUT_Y4M_1}" -q 100 -o "${ENCODED_FILE}"
"${AVIFENC}" -s 8 -q 100 "${INPUT_Y4M_0}" "${INPUT_Y4M_1}" -o "${ENCODED_FILE}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am worried about this change. Will the former order fail or maybe display a warning to the user after this change? Otherwise this is too dangerous to ignore the trailing flags in my opinion.

A solution to this could be "The first occurrence of each frame flag type also applies to all frames appearing before it in the command line". But this is complex so maybe there is a better solution.

Copy link
Contributor Author

@tongyuantongyu tongyuantongyu Aug 22, 2023

Choose a reason for hiding this comment

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

The first occurrence of each frame flag type also applies to all frames appearing before it in the command line

This would be too conterintuitive for new users not knowing the history of avifenc.
We may specially apply trailing flags if there's only one input file, but for multiple inputs case, there seems to have no way to be both intuitive and backward compatible. But at least we should print a warning when there are trailing flags. I'll add it.

Luckily we are still at v0.x. @wantehchang I see you are planning v1.0 release in #1509. This change may need to be included in v1.0 as well if we want it, or there probably won't be chance to do it later.

Copy link
Collaborator

Choose a reason for hiding this comment

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

After discussion with @wantehchang we decided to keep backward compatibility as much as possible, and thus this pull request can be merged after the libavif 1.0.0 release.

Instead of positional arguments, @wantehchang suggested frame-specific prefixes similar to the channel-specific prefixes used for the -a flag:

libavif/apps/avifenc.c

Lines 161 to 162 in 5e7a177

printf(" 2. color:<key>=<value> or c:<key>=<value> applies only to the color (YUV) planes.\n");
printf(" 3. alpha:<key>=<value> or a:<key>=<value> applies only to the alpha plane (if present).\n");

Would something like -q 3:75 work? Meaning -q 75 but applied to the frame with index 3 only.

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 would in theory work, but will make the command line really messy and complicated to parse:

  • I'd still place them right before target input so I don't need to count to understand what it does - but I still need to count when writing commands.
  • The parsing have to be two-pass.
  • Is "frame with index 3" "output frame index 3" or "input file index 3"? I don't have clear preference, but in case we end up needing both, it will be even more complicated than the --from-index approach I wrote before.
  • I'd prefer "3" means starting from "3" if we going through this way, so it maps precisely to a programmatic change to one of avifEncoder field.

However, now for the most common single input case, trailing options still have effect; we can also treat the trailing options as applying to the first input for multiple input case. I would say it's a really strange expectation, if someone is putting options in between inputs and expect it to apply to all inputs - even if avifenc actually works in this way.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you for the detailed analysis.

I'd still place them right before target input so I don't need to count to understand what it does - but I still need to count when writing commands.

-q next:75 syntax is fine with me, next to -q 3:75 syntax (for y4m input).

The parsing have to be two-pass.

I would say that it matters less than backward compatibility and user friendliness, unless it makes the code overly complex.

Is "frame with index 3" "output frame index 3" or "input file index 3"? I don't have clear preference, but in case we end up needing both, it will be even more complicated than the --from-index approach I wrote before.

I would go with "output frame index 3".

I'd prefer "3" means starting from "3" if we going through this way, so it maps precisely to a programmatic change to one of avifEncoder field.

-q 3+:75 syntax is fine with me (or something similar to signal that frames >=3 are targeted). It can coexist with -q 3:75 syntax if the latter is useful.

However, now for the most common single input case, trailing options still have effect; we can also treat the trailing options as applying to the first input for multiple input case. I would say it's a really strange expectation, if someone is putting options in between inputs and expect it to apply to all inputs - even if avifenc actually works in this way.

What about this precedence order, for each frame:

  1. Apply the right-most frame-specific flag that targets at least this frame.
  2. If none, apply the right-most global flag.
  3. If none, apply default value.

Example: frame 3 will have final q75 for flags -q 3:73 -q 3+:74 -q 2+:75 -q 76

The most important part is keeping the current behavior for the currently possible ordered flag combinations. To prove that, it would be good to keep existing sh tests as is in this PR (lines can be added to them of course).

Copy link
Collaborator

Choose a reason for hiding this comment

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

@vigneshvg suggested using lists, for example -q 73,74,75 would set quality to 73 for the first frame, to 74 for the second frame and to 75 for all remaining frames.

I guess this only works if the delimiter , is not already used in a frame-specific flag. I do not see any in this PR, besides --scaling-mode H[,V] that was reduced to --scaling-mode H and removed, and -a,--advanced KEY[=VALUE] maybe. Does anyone know if a codec uses , in VALUE as of today or maybe in the future?

I'd still place them right before target input so I don't need to count to understand what it does

This comment was related to -q 3:75 but it is relevant here. How important is it to keep settings flags close to their corresponding input file path? How important is it to not have to count output frames? For progressive, it was assumed that the number of frames/layers was small enough to not pay attention to these constraints.


Let me know when this PR is ready for review.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

using lists, for example -q 73,74,75

That wants options have "set current" semantic, but we're sort of locked to "update" semantic:

There's the --advanced option, which in general we can't (or don't know how to) unset. Set for current frame means we need to unset it (revert to default) for the next frame, but we don't know what's the default so we can't do it. Therefore for --advanced we are restricted to do "update", and we'd better be consistent and make every option do "update" then.

I had noticed this restriction while adding the support for the library, but forgot the exact reason why we'd do "update" for the command as well, so I was talking about being consistent with using library.

I guess this only works if the delimiter , is not already used in a frame-specific flag.

Not likely to happen, but if someone decide to use comma in their filename: --advanced film-grain-table=file,name.tbl (maybe with a more reasonable name).

How important is it to keep settings flags close to their corresponding input file path? How important is it to not have to count output frames?

It's more about being able to copypaste / reusing part of the options.

Say you come up with some adaptive logic to generate progressive images:

  • For all images we have a quick preview layer and a full layer.
  • If the image is large, insert one more layer in between.

With the current approach, you can have a command segment for each layer, and just join the parts of layers that you decide to have. With explicit index or comma/somewhat separated list, you can't to this and have to assemble whole command for each case separately.

Also during experiment, that will need more modifications / more scattered modifications to the command to add / remove layers (that is not the last one, which is the usual case).

Let me know when this PR is ready for review.

Oh It's ready now. Sorry for this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's more about being able to copypaste / reusing part of the options.

Say you come up with some adaptive logic to generate progressive images:

  • For all images we have a quick preview layer and a full layer.
  • If the image is large, insert one more layer in between.

With the current approach, you can have a command segment for each layer, and just join the parts of layers that you decide to have. With explicit index or comma/somewhat separated list, you can't to this and have to assemble whole command for each case separately.

In this case i don't think there is a big difference between the "list" syntax and a "positional" syntax. Either way you have to assemble the arguments and combine them into one string. Whether you assemble them as "-q 1,2,3 file1 file2 file3" or "-q 1 file1 -q 2 file2 -q 3 file3" does not really matter.

Also during experiment, that will need more modifications / more scattered modifications to the command to add / remove layers (that is not the last one, which is the usual case).

For the progressive images case, since the number of inputs is limited (if i understand correctly, there can only be a maximum of 4 inputs for 4 layers), i really think the list syntax is the simplest way to go rather than introducing positional arguments and creating backwards incompatibility for the more general use-case (single layer non-progressive avif images).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess this only works if the delimiter , is not already used in a frame-specific flag.

I am not sure what you mean by a "frame-specific flag", but we already use comma as a delimiter for --crop, --clap and --pasp flags.

Also, the only other positional argument that i see in avifenc is the --duration flag which can be specified multiple times when creating animated AVIF images. The same flag can be repeated multiple times and it applies to all the following frames unless a new value is specified with another --duration flag.

We can consider doing the same thing with -q as well. But that still introduces backwards incompatibility for the wider use-case. So i am not in favor of it unless others feel strongly so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"-q 1 file1 -q 2 file2 -q 3 file3"

introducing positional arguments and creating backwards incompatibility for the more general use-case (single layer non-progressive avif images).

But that still introduces backwards incompatibility for the wider use-case.

"positional" behavior is enabled with the :u (or :update) suffix on the flag, so there's no compatibility issue now. That means -q:u 1 file1 -q:u 2 file2 -q:u 3 file3. -q 1 file1 -q 2 file2 -q 3 file3 would behave exactly the same as the current: only -q 3 is effective. (This PR add a warning message in this case).

In this case i don't think there is a big difference between the "list" syntax and a "positional" syntax.

Let's consider some cases:

With "positional" syntax, tweak each layer separately is easy:

LAYER1="-q:u 50 --scaling-mode:u 1/8 $INPUT_FILE"
LAYER2="-q:u 30 --scaling-mode:u 1/2 $INPUT_FILE"
LAYER3="-q:u 80 --scaling-mode:u 1 $INPUT_FILE"

if [ $INPUT_WIDTH -gt 1024 ]; do
    avifenc -j 8 $LAYER1 $LAYER2 $LAYER3 -o $OUTPUT_FILE
else
    avifenc -j 8 $LAYER1 $LAYER3 -o $OUTPUT_FILE
fi

With "list" syntax, you have to spell (and modify) some option values twice:

if [ $INPUT_WIDTH -gt 1024 ]; do
    avifenc -j 8 -q 50,30,80 --scaling-mode 1/8,1/2,1 $INPUT_FILE $INPUT_FILE $INPUT_FILE -o $OUTPUT_FILE
else
    avifenc -j 8 -q 50,80 --scaling-mode 1/8,1 $INPUT_FILE $INPUT_FILE -o $OUTPUT_FILE
fi

Otherwise you need to make 3 variables for each option, or 6 in total for this case.

And there's also boolean flag:

# Don't tile for the 1/8 size first layer
avifenc --some-global-options \
  -q:u 50 --scaling-mode:u 1/8 input.png \
  -q:u 80 --scaling-mode:u 1 --autotiling:u input.png \
  -o output.avif

For list syntax, that'll need --autotiling false,true. (or --no-autotiling,autotiling?)

And for --advanced, list syntax can be confusing:

# Looks like setting two options at once,
# but is actually setting denoise-noise-level=50 at first input, and c:sharpness=7 at second.
avifenc -a denoise-noise-level=50,sharpness=7 input.png input.png output.avif

Whereas in positional syntax, it's always clear:

avifenc --some-global-options \
  -a denoise-noise-level=50 input.png \
  -a:update sharpness=7 input.png \
  output.avif

tests/test_cmd_progressive.sh Outdated Show resolved Hide resolved
Copy link
Contributor Author

@tongyuantongyu tongyuantongyu left a comment

Choose a reason for hiding this comment

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

I'm still working on the split, and I'll update CHANGES.md after the PR is in good shape.

apps/avifenc.c Outdated Show resolved Hide resolved
apps/avifenc.c Show resolved Hide resolved
apps/avifenc.c Outdated Show resolved Hide resolved
apps/avifenc.c Outdated Show resolved Hide resolved
apps/avifenc.c Outdated Show resolved Hide resolved
apps/avifenc.c Outdated Show resolved Hide resolved
apps/avifenc.c Outdated Show resolved Hide resolved
apps/avifenc.c Outdated Show resolved Hide resolved
@@ -60,14 +61,24 @@ pushd ${TMP_DIR}
"${ARE_IMAGES_EQUAL}" "${INPUT_Y4M_0}" "${DECODED_FILE}" 0 && exit 1

# Lossless test.
"${AVIFENC}" -s 8 "${INPUT_Y4M_0}" "${INPUT_Y4M_1}" -q 100 -o "${ENCODED_FILE}"
"${AVIFENC}" -s 8 -q 100 "${INPUT_Y4M_0}" "${INPUT_Y4M_1}" -o "${ENCODED_FILE}"
Copy link
Contributor Author

@tongyuantongyu tongyuantongyu Aug 22, 2023

Choose a reason for hiding this comment

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

The first occurrence of each frame flag type also applies to all frames appearing before it in the command line

This would be too conterintuitive for new users not knowing the history of avifenc.
We may specially apply trailing flags if there's only one input file, but for multiple inputs case, there seems to have no way to be both intuitive and backward compatible. But at least we should print a warning when there are trailing flags. I'll add it.

Luckily we are still at v0.x. @wantehchang I see you are planning v1.0 release in #1509. This change may need to be included in v1.0 as well if we want it, or there probably won't be chance to do it later.

Copy link
Contributor Author

@tongyuantongyu tongyuantongyu left a comment

Choose a reason for hiding this comment

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

Still WIP.

apps/avifenc.c Show resolved Hide resolved
@@ -60,14 +61,24 @@ pushd ${TMP_DIR}
"${ARE_IMAGES_EQUAL}" "${INPUT_Y4M_0}" "${DECODED_FILE}" 0 && exit 1

# Lossless test.
"${AVIFENC}" -s 8 "${INPUT_Y4M_0}" "${INPUT_Y4M_1}" -q 100 -o "${ENCODED_FILE}"
"${AVIFENC}" -s 8 -q 100 "${INPUT_Y4M_0}" "${INPUT_Y4M_1}" -o "${ENCODED_FILE}"
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 would in theory work, but will make the command line really messy and complicated to parse:

  • I'd still place them right before target input so I don't need to count to understand what it does - but I still need to count when writing commands.
  • The parsing have to be two-pass.
  • Is "frame with index 3" "output frame index 3" or "input file index 3"? I don't have clear preference, but in case we end up needing both, it will be even more complicated than the --from-index approach I wrote before.
  • I'd prefer "3" means starting from "3" if we going through this way, so it maps precisely to a programmatic change to one of avifEncoder field.

However, now for the most common single input case, trailing options still have effect; we can also treat the trailing options as applying to the first input for multiple input case. I would say it's a really strange expectation, if someone is putting options in between inputs and expect it to apply to all inputs - even if avifenc actually works in this way.

apps/avifenc.c Outdated Show resolved Hide resolved
apps/avifenc.c Outdated Show resolved Hide resolved
Copy link
Collaborator

@y-guyon y-guyon left a comment

Choose a reason for hiding this comment

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

Getting there!

tests/test_cmd.sh Outdated Show resolved Hide resolved
apps/avifenc.c Outdated Show resolved Hide resolved
apps/avifenc.c Outdated Show resolved Hide resolved
printf(" -- : Signals the end of options. Everything after this is interpreted as file names.\n");
printf("\n");
printf(" The following options can optionally have a :u (or :update) suffix like -q:u, to apply only to input files appearing after the option:\n");
printf(" -q,--qcolor Q : Set quality for color (%d-%d, where %d is lossless)\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 am wondering if it should be -q[:u],--qcolor[:update] Q. It is definitely heavier, but I would not be surprised if most users missed the sentence above without a reminder in each option syntax description.

A clearer but lengthy alternative would be another line such as

printf(" -q:u,--qcolor:update Q : Same as -q Q but only applies to input files appearing after the option\n");

for every frame-specific options.

What do you think? We can also consider :u to be so niche that more than a sentence is not worth it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We all accept :u will mostly be used for progressive AVIF encoding. Progressive in AVIF is quite different from other formats and what users familiar with, so it well deserves an "AVIF Progressive" Wiki page, and we can have the :u explained in detail there.

apps/avifenc.c Outdated Show resolved Hide resolved
apps/avifenc.c Outdated Show resolved Hide resolved
apps/avifenc.c Outdated

if (fileSettings->tileColsLog2.set || fileSettings->tileRowsLog2.set) {
// If this file has manual tile config set, disable autotiling;
fileSettings->autoTiling = boolSettingsEntryOf(AVIF_FALSE);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is already handled as an error at line 1856

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 solving a rather weird situation if someone was using autotiling for the first input and decided to switch to manual tiling later.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please fix. Unless I missed something, using --autotiling will set fileSettings->tileRowsLog2 at line 1873, so the condition (fileSettings->tileColsLog2.set || fileSettings->tileRowsLog2.set) at line 1910 is true, so fileSettings->autoTiling is set to boolSettingsEntryOf(AVIF_FALSE).

Maybe there should be some test_cmd_autotiling.sh in another PR.

apps/avifenc.c Outdated Show resolved Hide resolved
fileSettings->autoTiling = boolSettingsEntryOf(AVIF_FALSE);
}
if (!fileSettings->tileRowsLog2.set) {
fileSettings->tileRowsLog2 = intSettingsEntryOf(0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

pendingSettings is memset() to 0 so this should be unnecessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this is more for a peace of mind that later code reading settings of first input won't see the entry's set field is AVIF_FALSE. It may appears confusing during debugging.

apps/avifenc.c Show resolved Hide resolved
Copy link
Collaborator

@y-guyon y-guyon left a comment

Choose a reason for hiding this comment

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

This looks good overall, thanks for your patience.

apps/avifenc.c Outdated Show resolved Hide resolved
apps/avifenc.c Outdated

if (fileSettings->tileColsLog2.set || fileSettings->tileRowsLog2.set) {
// If this file has manual tile config set, disable autotiling;
fileSettings->autoTiling = boolSettingsEntryOf(AVIF_FALSE);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please fix. Unless I missed something, using --autotiling will set fileSettings->tileRowsLog2 at line 1873, so the condition (fileSettings->tileColsLog2.set || fileSettings->tileRowsLog2.set) at line 1910 is true, so fileSettings->autoTiling is set to boolSettingsEntryOf(AVIF_FALSE).

Maybe there should be some test_cmd_autotiling.sh in another PR.

apps/avifenc.c Outdated
returnCode = 1;
goto cleanup;
}
// At here this entry can only be set by command line, so its value have to be AVIF_TRUE.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I had a hard time understanding this sentence, maybe make it extra verbose?

assert(fileSettings->autoTiling.value);  // There is no --noautotiling flag.
// Disable manual tiling (possibly set by previous input).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Put checks on tiling paramters together (should be from the beginning but was somewhat missed), and added some comments. Hope this explains it better.

In general this (switching between auto and manual tiling during encoding) should be a very rare case.

Copy link
Contributor Author

@tongyuantongyu tongyuantongyu left a comment

Choose a reason for hiding this comment

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

Just realized replies from last round of review was still in pending state. @y-guyon sorry for the confusion.

printf(" -- : Signals the end of options. Everything after this is interpreted as file names.\n");
printf("\n");
printf(" The following options can optionally have a :u (or :update) suffix like -q:u, to apply only to input files appearing after the option:\n");
printf(" -q,--qcolor Q : Set quality for color (%d-%d, where %d is lossless)\n",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We all accept :u will mostly be used for progressive AVIF encoding. Progressive in AVIF is quite different from other formats and what users familiar with, so it well deserves an "AVIF Progressive" Wiki page, and we can have the :u explained in detail there.


static avifBool strpre(const char * str, const char * prefix)
{
return strncmp(str, prefix, strlen(prefix)) == 0;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The third parameter is the "maximum number of characters to compare", and "Characters following the null character are not compared": https://en.cppreference.com/w/c/string/byte/strncmp, so this should be fine.

tests/test_cmd.sh Outdated Show resolved Hide resolved
apps/avifenc.c Outdated Show resolved Hide resolved
apps/avifenc.c Outdated Show resolved Hide resolved
apps/avifenc.c Outdated

if (fileSettings->tileColsLog2.set || fileSettings->tileRowsLog2.set) {
// If this file has manual tile config set, disable autotiling;
fileSettings->autoTiling = boolSettingsEntryOf(AVIF_FALSE);
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 solving a rather weird situation if someone was using autotiling for the first input and decided to switch to manual tiling later.

fileSettings->autoTiling = boolSettingsEntryOf(AVIF_FALSE);
}
if (!fileSettings->tileRowsLog2.set) {
fileSettings->tileRowsLog2 = intSettingsEntryOf(0);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this is more for a peace of mind that later code reading settings of first input won't see the entry's set field is AVIF_FALSE. It may appears confusing during debugging.

apps/avifenc.c Show resolved Hide resolved
apps/avifenc.c Outdated Show resolved Hide resolved
apps/avifenc.c Outdated
returnCode = 1;
goto cleanup;
}
// At here this entry can only be set by command line, so its value have to be AVIF_TRUE.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Put checks on tiling paramters together (should be from the beginning but was somewhat missed), and added some comments. Hope this explains it better.

In general this (switching between auto and manual tiling during encoding) should be a very rare case.

Copy link
Collaborator

@y-guyon y-guyon left a comment

Choose a reason for hiding this comment

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

I forgot: could you add an entry to CHANGELOG.md? The following should be enough:

### Added
...
+* Add avifenc flag suffixes ":update" and ":u". Quality-relative,
+  tiling-relative and codec-specific flags can now be positional, relative to
+  input files.

@y-guyon y-guyon merged commit 4efa658 into AOMediaCodec:main Sep 5, 2023
14 checks passed
@tongyuantongyu tongyuantongyu deleted the avifenc_change_config branch September 13, 2023 12:34
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.

4 participants