diff mbox series

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

Message ID 20200728122202.14074-1-y.takio@gmail.com
State New
Headers show
Series [FFmpeg-devel] libavcodec/libx264.c: Fix chromaoffset of libx264 doesn't work
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Takio Yamaoka July 28, 2020, 12:22 p.m. UTC
An initial value of `AVCodecContext::chromaoffset` is zero,
then it causes to block `-chromaoffset` setting as result.
In addition, even though a negative number of `chromaoffset`
is meaningful, `X264Context::chroma_offset` is initialized
with `-1` as no setting and ignored if it is negative number.

To fix above, it changes ...
- a value of `X264Context::chroma_offset` to 0 as no setting
    - due to x264's default value
- conditional statement to import `-chromaoffset`

Signed-off-by: Takio Yamaoka <y.takio@gmail.com>
---
 libavcodec/libx264.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Takio Yamaoka July 28, 2020, 12:42 p.m. UTC | #1
I forgot to sign-off to patch, so I've resent the same patch.
This patch is bug fix to handle chromaoffset parameter for libx264.

Best Regards,
Takio

2020年7月28日(火) 21:22 Takio Yamaoka <y.takio@gmail.com>:
>
> An initial value of `AVCodecContext::chromaoffset` is zero,
> then it causes to block `-chromaoffset` setting as result.
> In addition, even though a negative number of `chromaoffset`
> is meaningful, `X264Context::chroma_offset` is initialized
> with `-1` as no setting and ignored if it is negative number.
>
> To fix above, it changes ...
> - a value of `X264Context::chroma_offset` to 0 as no setting
>     - due to x264's default value
> - conditional statement to import `-chromaoffset`
>
> Signed-off-by: Takio Yamaoka <y.takio@gmail.com>
> ---
>  libavcodec/libx264.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
> index 7bbeab7d4c..347d29df27 100644
> --- a/libavcodec/libx264.c
> +++ b/libavcodec/libx264.c
> @@ -681,11 +681,11 @@ static av_cold int X264_init(AVCodecContext *avctx)
>
>  #if FF_API_PRIVATE_OPT
>  FF_DISABLE_DEPRECATION_WARNINGS
> -    if (avctx->chromaoffset >= 0)
> +    if (avctx->chromaoffset)
>          x4->chroma_offset = avctx->chromaoffset;
>  FF_ENABLE_DEPRECATION_WARNINGS
>  #endif
> -    if (x4->chroma_offset >= 0)
> +    if (x4->chroma_offset)
>          x4->params.analyse.i_chroma_qp_offset = x4->chroma_offset;
>
>      if (avctx->gop_size >= 0)
> @@ -1140,7 +1140,7 @@ static const AVOption options[] = {
>      { "vlc",              NULL, 0, AV_OPT_TYPE_CONST, { .i64 = 0 },  INT_MIN, INT_MAX, VE, "coder" },
>      { "ac",               NULL, 0, AV_OPT_TYPE_CONST, { .i64 = 1 },  INT_MIN, INT_MAX, VE, "coder" },
>      { "b_strategy",   "Strategy to choose between I/P/B-frames",          OFFSET(b_frame_strategy), AV_OPT_TYPE_INT, { .i64 = -1 }, -1, 2, VE },
> -    { "chromaoffset", "QP difference between chroma and luma",            OFFSET(chroma_offset), AV_OPT_TYPE_INT, { .i64 = -1 }, INT_MIN, INT_MAX, VE },
> +    { "chromaoffset", "QP difference between chroma and luma",            OFFSET(chroma_offset), AV_OPT_TYPE_INT, { .i64 = 0 }, INT_MIN, INT_MAX, VE },
>      { "sc_threshold", "Scene change threshold",                           OFFSET(scenechange_threshold), AV_OPT_TYPE_INT, { .i64 = -1 }, INT_MIN, INT_MAX, VE },
>      { "noise_reduction", "Noise reduction",                               OFFSET(noise_reduction), AV_OPT_TYPE_INT, { .i64 = -1 }, INT_MIN, INT_MAX, VE },
>
> --
> 2.17.1
>
Takio Yamaoka Aug. 3, 2020, 2:14 a.m. UTC | #2
Hi devel,
Is there anyone who can review this?

Thanks,
Takio

2020年7月28日(火) 21:42 Takio Yamaoka <y.takio@gmail.com>:
>
> I forgot to sign-off to patch, so I've resent the same patch.
> This patch is bug fix to handle chromaoffset parameter for libx264.
>
> Best Regards,
> Takio
>
> 2020年7月28日(火) 21:22 Takio Yamaoka <y.takio@gmail.com>:
> >
> > An initial value of `AVCodecContext::chromaoffset` is zero,
> > then it causes to block `-chromaoffset` setting as result.
> > In addition, even though a negative number of `chromaoffset`
> > is meaningful, `X264Context::chroma_offset` is initialized
> > with `-1` as no setting and ignored if it is negative number.
> >
> > To fix above, it changes ...
> > - a value of `X264Context::chroma_offset` to 0 as no setting
> >     - due to x264's default value
> > - conditional statement to import `-chromaoffset`
> >
> > Signed-off-by: Takio Yamaoka <y.takio@gmail.com>
> > ---
> >  libavcodec/libx264.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
> > index 7bbeab7d4c..347d29df27 100644
> > --- a/libavcodec/libx264.c
> > +++ b/libavcodec/libx264.c
> > @@ -681,11 +681,11 @@ static av_cold int X264_init(AVCodecContext *avctx)
> >
> >  #if FF_API_PRIVATE_OPT
> >  FF_DISABLE_DEPRECATION_WARNINGS
> > -    if (avctx->chromaoffset >= 0)
> > +    if (avctx->chromaoffset)
> >          x4->chroma_offset = avctx->chromaoffset;
> >  FF_ENABLE_DEPRECATION_WARNINGS
> >  #endif
> > -    if (x4->chroma_offset >= 0)
> > +    if (x4->chroma_offset)
> >          x4->params.analyse.i_chroma_qp_offset = x4->chroma_offset;
> >
> >      if (avctx->gop_size >= 0)
> > @@ -1140,7 +1140,7 @@ static const AVOption options[] = {
> >      { "vlc",              NULL, 0, AV_OPT_TYPE_CONST, { .i64 = 0 },  INT_MIN, INT_MAX, VE, "coder" },
> >      { "ac",               NULL, 0, AV_OPT_TYPE_CONST, { .i64 = 1 },  INT_MIN, INT_MAX, VE, "coder" },
> >      { "b_strategy",   "Strategy to choose between I/P/B-frames",          OFFSET(b_frame_strategy), AV_OPT_TYPE_INT, { .i64 = -1 }, -1, 2, VE },
> > -    { "chromaoffset", "QP difference between chroma and luma",            OFFSET(chroma_offset), AV_OPT_TYPE_INT, { .i64 = -1 }, INT_MIN, INT_MAX, VE },
> > +    { "chromaoffset", "QP difference between chroma and luma",            OFFSET(chroma_offset), AV_OPT_TYPE_INT, { .i64 = 0 }, INT_MIN, INT_MAX, VE },
> >      { "sc_threshold", "Scene change threshold",                           OFFSET(scenechange_threshold), AV_OPT_TYPE_INT, { .i64 = -1 }, INT_MIN, INT_MAX, VE },
> >      { "noise_reduction", "Noise reduction",                               OFFSET(noise_reduction), AV_OPT_TYPE_INT, { .i64 = -1 }, INT_MIN, INT_MAX, VE },
> >
> > --
> > 2.17.1
> >
Jan Ekström Aug. 4, 2020, 8:59 p.m. UTC | #3
On Tue, Jul 28, 2020 at 3:30 PM Takio Yamaoka <y.takio@gmail.com> wrote:
>
> An initial value of `AVCodecContext::chromaoffset` is zero,
> then it causes to block `-chromaoffset` setting as result.
> In addition, even though a negative number of `chromaoffset`
> is meaningful, `X264Context::chroma_offset` is initialized
> with `-1` as no setting and ignored if it is negative number.
>
> To fix above, it changes ...
> - a value of `X264Context::chroma_offset` to 0 as no setting
>     - due to x264's default value
> - conditional statement to import `-chromaoffset`
>
> Signed-off-by: Takio Yamaoka <y.takio@gmail.com>
> ---
>  libavcodec/libx264.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
> index 7bbeab7d4c..347d29df27 100644
> --- a/libavcodec/libx264.c
> +++ b/libavcodec/libx264.c
> @@ -681,11 +681,11 @@ static av_cold int X264_init(AVCodecContext *avctx)
>
>  #if FF_API_PRIVATE_OPT
>  FF_DISABLE_DEPRECATION_WARNINGS
> -    if (avctx->chromaoffset >= 0)
> +    if (avctx->chromaoffset)
>          x4->chroma_offset = avctx->chromaoffset;
>  FF_ENABLE_DEPRECATION_WARNINGS
>  #endif
> -    if (x4->chroma_offset >= 0)
> +    if (x4->chroma_offset)
>          x4->params.analyse.i_chroma_qp_offset = x4->chroma_offset;
>
>      if (avctx->gop_size >= 0)
> @@ -1140,7 +1140,7 @@ static const AVOption options[] = {
>      { "vlc",              NULL, 0, AV_OPT_TYPE_CONST, { .i64 = 0 },  INT_MIN, INT_MAX, VE, "coder" },
>      { "ac",               NULL, 0, AV_OPT_TYPE_CONST, { .i64 = 1 },  INT_MIN, INT_MAX, VE, "coder" },
>      { "b_strategy",   "Strategy to choose between I/P/B-frames",          OFFSET(b_frame_strategy), AV_OPT_TYPE_INT, { .i64 = -1 }, -1, 2, VE },
> -    { "chromaoffset", "QP difference between chroma and luma",            OFFSET(chroma_offset), AV_OPT_TYPE_INT, { .i64 = -1 }, INT_MIN, INT_MAX, VE },
> +    { "chromaoffset", "QP difference between chroma and luma",            OFFSET(chroma_offset), AV_OPT_TYPE_INT, { .i64 = 0 }, INT_MIN, INT_MAX, VE },
>      { "sc_threshold", "Scene change threshold",                           OFFSET(scenechange_threshold), AV_OPT_TYPE_INT, { .i64 = -1 }, INT_MIN, INT_MAX, VE },
>      { "noise_reduction", "Noise reduction",                               OFFSET(noise_reduction), AV_OPT_TYPE_INT, { .i64 = -1 }, INT_MIN, INT_MAX, VE },
>

General change looks alright to me, after checking the following points.
- AVCodecContext's chromaoffset indeed defaults to 0 through the
DEFAULT define (#define DEFAULT 0 //should be NAN but it does not work
as it is not a constant in glibc as required by ANSI/ISO C)
- x264 default is 0 (thus we thankfully haven't overwritten this
before, and the patch doesn't lead to a change in that behavior,
either)
- value in x264 can be between -12 and 12, thus both positive and
negative values indeed can be set
(x264_clip3(h->param.analyse.i_chroma_qp_offset, -12, 12) can be found
in x264's code)

Best regards,
Jan
Takio Yamaoka Aug. 5, 2020, 12:55 a.m. UTC | #4
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

2020年8月5日(水) 6:00 Jan Ekström <jeebjp@gmail.com>:
>
> On Tue, Jul 28, 2020 at 3:30 PM Takio Yamaoka <y.takio@gmail.com> wrote:
> >
> > An initial value of `AVCodecContext::chromaoffset` is zero,
> > then it causes to block `-chromaoffset` setting as result.
> > In addition, even though a negative number of `chromaoffset`
> > is meaningful, `X264Context::chroma_offset` is initialized
> > with `-1` as no setting and ignored if it is negative number.
> >
> > To fix above, it changes ...
> > - a value of `X264Context::chroma_offset` to 0 as no setting
> >     - due to x264's default value
> > - conditional statement to import `-chromaoffset`
> >
> > Signed-off-by: Takio Yamaoka <y.takio@gmail.com>
> > ---
> >  libavcodec/libx264.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
> > index 7bbeab7d4c..347d29df27 100644
> > --- a/libavcodec/libx264.c
> > +++ b/libavcodec/libx264.c
> > @@ -681,11 +681,11 @@ static av_cold int X264_init(AVCodecContext *avctx)
> >
> >  #if FF_API_PRIVATE_OPT
> >  FF_DISABLE_DEPRECATION_WARNINGS
> > -    if (avctx->chromaoffset >= 0)
> > +    if (avctx->chromaoffset)
> >          x4->chroma_offset = avctx->chromaoffset;
> >  FF_ENABLE_DEPRECATION_WARNINGS
> >  #endif
> > -    if (x4->chroma_offset >= 0)
> > +    if (x4->chroma_offset)
> >          x4->params.analyse.i_chroma_qp_offset = x4->chroma_offset;
> >
> >      if (avctx->gop_size >= 0)
> > @@ -1140,7 +1140,7 @@ static const AVOption options[] = {
> >      { "vlc",              NULL, 0, AV_OPT_TYPE_CONST, { .i64 = 0 },  INT_MIN, INT_MAX, VE, "coder" },
> >      { "ac",               NULL, 0, AV_OPT_TYPE_CONST, { .i64 = 1 },  INT_MIN, INT_MAX, VE, "coder" },
> >      { "b_strategy",   "Strategy to choose between I/P/B-frames",          OFFSET(b_frame_strategy), AV_OPT_TYPE_INT, { .i64 = -1 }, -1, 2, VE },
> > -    { "chromaoffset", "QP difference between chroma and luma",            OFFSET(chroma_offset), AV_OPT_TYPE_INT, { .i64 = -1 }, INT_MIN, INT_MAX, VE },
> > +    { "chromaoffset", "QP difference between chroma and luma",            OFFSET(chroma_offset), AV_OPT_TYPE_INT, { .i64 = 0 }, INT_MIN, INT_MAX, VE },
> >      { "sc_threshold", "Scene change threshold",                           OFFSET(scenechange_threshold), AV_OPT_TYPE_INT, { .i64 = -1 }, INT_MIN, INT_MAX, VE },
> >      { "noise_reduction", "Noise reduction",                               OFFSET(noise_reduction), AV_OPT_TYPE_INT, { .i64 = -1 }, INT_MIN, INT_MAX, VE },
> >
>
> General change looks alright to me, after checking the following points.
> - AVCodecContext's chromaoffset indeed defaults to 0 through the
> DEFAULT define (#define DEFAULT 0 //should be NAN but it does not work
> as it is not a constant in glibc as required by ANSI/ISO C)
> - x264 default is 0 (thus we thankfully haven't overwritten this
> before, and the patch doesn't lead to a change in that behavior,
> either)
> - value in x264 can be between -12 and 12, thus both positive and
> negative values indeed can be set
> (x264_clip3(h->param.analyse.i_chroma_qp_offset, -12, 12) can be found
> in x264's code)
>
> Best regards,
> Jan
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Jan Ekström Aug. 5, 2020, 10:23 p.m. UTC | #5
On Wed, Aug 5, 2020 at 4:27 AM Takio Yamaoka <y.takio@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@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.
Takio Yamaoka Aug. 6, 2020, 12:16 a.m. UTC | #6
2020年8月6日(木) 7:24 Jan Ekström <jeebjp@gmail.com>:
>
> Sorry, decided to take a look at the logic but didn't have the time to
> actually test it locally.

Thank you for replying. It sounds good for me.

> 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 .

Yes, Thanks. I did the same thing.
So, I should pasted test results and history of this. I'm sorry for
bothering you.

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

Of course! Thank you for correcting me.
I have learned a lot from this commit log.

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

Oh, I'm sorry that my email must be difficult to read...
I greatly appreciate your advice. I'm going to follow these rules.
#Is it fine in the above replying?

Anyway Thank you.

Best Regards,
Takio
Jan Ekström Aug. 6, 2020, 10:15 a.m. UTC | #7
On Thu, Aug 6, 2020 at 3:16 AM Takio Yamaoka <y.takio@gmail.com> wrote:
>
> 2020年8月6日(木) 7:24 Jan Ekström <jeebjp@gmail.com>:
> >
> > Sorry, decided to take a look at the logic but didn't have the time to
> > actually test it locally.
>
> Thank you for replying. It sounds good for me.
>
> > 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 .
>
> Yes, Thanks. I did the same thing.
> So, I should pasted test results and history of this. I'm sorry for
> bothering you.
>
> > If you are OK with the following rewording of the commit message, I
> > can push it tomorrow after work:
>
> Of course! Thank you for correcting me.
> I have learned a lot from this commit log.
>

Cheers. Applied as cc6c56f5d900f86167f6592215e613a69341858a .

Thanks for noticing that this was broken and sending out a patch.

> > P.S. Top-posting is frowned upon on this mailing list, so please reply
> > in-line or underneath in the future, if possible.
>
> Oh, I'm sorry that my email must be difficult to read...
> I greatly appreciate your advice. I'm going to follow these rules.
> #Is it fine in the above replying?
>

I do not consider it too difficult to read as it's the more common way
people utilize e-mail these days, but I just decided to note the
practice on this mailing list before someone else did it in less nice
words ;)

Jan
Takio Yamaoka Aug. 6, 2020, 3:29 p.m. UTC | #8
2020年8月6日(木) 19:39 Jan Ekström <jeebjp@gmail.com>:
>
> Cheers. Applied as cc6c56f5d900f86167f6592215e613a69341858a .
>
> Thanks for noticing that this was broken and sending out a patch.

It's my pleasure!

> > > P.S. Top-posting is frowned upon on this mailing list, so please reply
> > > in-line or underneath in the future, if possible.
> >
> > Oh, I'm sorry that my email must be difficult to read...
> > I greatly appreciate your advice. I'm going to follow these rules.
> > #Is it fine in the above replying?
> >
>
> I do not consider it too difficult to read as it's the more common way
> people utilize e-mail these days, but I just decided to note the
> practice on this mailing list before someone else did it in less nice
> words ;)

Thank you for your kindness :)
Takio
diff mbox series

Patch

diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
index 7bbeab7d4c..347d29df27 100644
--- a/libavcodec/libx264.c
+++ b/libavcodec/libx264.c
@@ -681,11 +681,11 @@  static av_cold int X264_init(AVCodecContext *avctx)
 
 #if FF_API_PRIVATE_OPT
 FF_DISABLE_DEPRECATION_WARNINGS
-    if (avctx->chromaoffset >= 0)
+    if (avctx->chromaoffset)
         x4->chroma_offset = avctx->chromaoffset;
 FF_ENABLE_DEPRECATION_WARNINGS
 #endif
-    if (x4->chroma_offset >= 0)
+    if (x4->chroma_offset)
         x4->params.analyse.i_chroma_qp_offset = x4->chroma_offset;
 
     if (avctx->gop_size >= 0)
@@ -1140,7 +1140,7 @@  static const AVOption options[] = {
     { "vlc",              NULL, 0, AV_OPT_TYPE_CONST, { .i64 = 0 },  INT_MIN, INT_MAX, VE, "coder" },
     { "ac",               NULL, 0, AV_OPT_TYPE_CONST, { .i64 = 1 },  INT_MIN, INT_MAX, VE, "coder" },
     { "b_strategy",   "Strategy to choose between I/P/B-frames",          OFFSET(b_frame_strategy), AV_OPT_TYPE_INT, { .i64 = -1 }, -1, 2, VE },
-    { "chromaoffset", "QP difference between chroma and luma",            OFFSET(chroma_offset), AV_OPT_TYPE_INT, { .i64 = -1 }, INT_MIN, INT_MAX, VE },
+    { "chromaoffset", "QP difference between chroma and luma",            OFFSET(chroma_offset), AV_OPT_TYPE_INT, { .i64 = 0 }, INT_MIN, INT_MAX, VE },
     { "sc_threshold", "Scene change threshold",                           OFFSET(scenechange_threshold), AV_OPT_TYPE_INT, { .i64 = -1 }, INT_MIN, INT_MAX, VE },
     { "noise_reduction", "Noise reduction",                               OFFSET(noise_reduction), AV_OPT_TYPE_INT, { .i64 = -1 }, INT_MIN, INT_MAX, VE },