[FFmpeg-devel] [PATCH] libavcodec/libx264.c: Fix chromaoffset of libx264 doesn't work

[Jan Ekström]

On Wed, Aug 5, 2020 at 4:27 AM Takio Yamaoka <y.takio at gmail.com> wrote:
>
> Thank you for the review!
> It is my first time to send a patch. So I was relieved to hear that.
>
> Is it OK to wait to merge?
>
> Best Regards,
> Takio
>

Sorry, decided to take a look at the logic but didn't have the time to
actually test it locally.

I have now tested it locally and it works as expected with the
following test cases:
<unset> and chromaoffset 0 (still kept at default, which is zero with x264)
chromaoffset -3
chromaoffset 5

Looking at the history, it was originally implemented correctly in
5764d38173661c29d954711dd5abfddf709e9ba4 but then broken in
b340bd8a58c32453172404a8e4240e3317e341da .

If you are OK with the following rewording of the commit message, I
can push it tomorrow after work:

avcodec/libx264: fix chroma quantizer offset usage

The default for the chromaoffset field in AVCodecContext
is zero, which until now always ended up overriding the
AVOption-set value, thus leading to the AVOption not working.

Additionally, the previous usage prevented the usage of
negative values, while both the variable as well as x264's
API would successfully handle such.

Thus, the default value of the AVOption is changed to match
the default of x264 (and what is currently the default for
the AVCodecContext chromaoffset field), and the checks are
changed to check for nonzero values.

This way:
1. the library default is still utilized if the value is zero.
2. both negative and positive values are correctly passed to
   x264.

For historical context, this was initially similarly
implemented in 5764d38173661c29d954711dd5abfddf709e9ba4, and
then b340bd8a58c32453172404a8e4240e3317e341da broke the
value.

Partially reverts commit b340bd8a58c32453172404a8e4240e3317e341da.

Signed-off-by: Takio Yamaoka <y.takio at gmail.com>

Best regards,
Jan

P.S. Top-posting is frowned upon on this mailing list, so please reply
in-line or underneath in the future, if possible.