| 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 | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
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 >
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 > >
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
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".
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.
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
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
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 --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 },
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(-)