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 <jee...@gmail.com>:
>
> On Tue, Jul 28, 2020 at 3:30 PM Takio Yamaoka <y.ta...@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.ta...@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-requ...@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
