Re: [libav-devel] [PATCH V2] lavc/qsvenc: add an option to disable MFE mode

[Li, Zhong]

Thanks for Mayxm/Luca/Diego's review. : ) 

> -Original Message-
> From: libav-devel [mailto:libav-devel-boun...@libav.org] On Behalf Of
> Maxym Dmytrychenko
> Sent: Tuesday, May 22, 2018 8:53 PM
> To: libav development 
> Subject: Re: [libav-devel] [PATCH V2] lavc/qsvenc: add an option to disable
> MFE mode
> 
> thanks Luca,
> 
> this patch should be reasonable
> 
> On Mon, May 21, 2018 at 9:58 AM, Luca Barbato 
> wrote:
> 
> > On 21/05/2018 08:33, Zhong Li wrote:
> > > Not convenient if using numerals to set MFE mode. It is ambiguous
> > > and misleading (e.g: user may misunderstand setting mfmode to 1 is
> > > to enable MFE but actually it is to disable MFE, and set it to be 5
> > > or
> > above is meaningless).
> > >
> > > V2: remove the manual option since it is not supported now.
> > >
> > > Signed-off-by: Zhong Li 
> > > ---
> > >  libavcodec/qsvenc_h264.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/libavcodec/qsvenc_h264.c b/libavcodec/qsvenc_h264.c
> > > index ae00ff8..2ecdb10 100644
> > > --- a/libavcodec/qsvenc_h264.c
> > > +++ b/libavcodec/qsvenc_h264.c
> > > @@ -94,7 +94,9 @@ static const AVOption options[] = {
> > >  { "aud", "Insert the Access Unit Delimiter NAL",
> > > OFFSET(qsv.aud),
> > AV_OPT_TYPE_INT, { .i64 = 0 }, 0, 1, VE},
> > >
> > >  #if QSV_HAVE_MF
> > > -{ "mfmode", "Multi-Frame Mode", OFFSET(qsv.mfmode),
> > AV_OPT_TYPE_INT, { .i64 = MFX_MF_AUTO }, 0, INT_MAX, VE },
> > > +{ "mfmode", "Multi-Frame Mode", OFFSET(qsv.mfmode),
> > AV_OPT_TYPE_INT, { .i64 = MFX_MF_AUTO }, MFX_MF_DEFAULT,
> MFX_MF_AUTO,
> > VE, "mfmode"},
> > > +{ "off", NULL, 0, AV_OPT_TYPE_CONST, { .i64 =
> MFX_MF_DISABLED
> > }, INT_MIN, INT_MAX, VE, "mfmode" },
> > > +{ "auto"   , NULL, 0, AV_OPT_TYPE_CONST, { .i64 =
> MFX_MF_AUTO
> >  }, INT_MIN, INT_MAX, VE, "mfmode" },
> > >  #endif
> > >
> > >  { NULL },
> > >
> >
> > Sounds fine to me, the previous iteration was on hold since Maxym
> > wanted to test it.
> >
> > I guess this time it should work as intended on every current mfx
> > release
> > :)
> >
> > lu
> > ___
> > libav-devel mailing list
> > libav-devel@libav.org
> > https://lists.libav.org/mailman/listinfo/libav-devel
> ___
> libav-devel mailing list
> libav-devel@libav.org
> https://lists.libav.org/mailman/listinfo/libav-devel
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH V2] lavc/qsvenc: add an option to disable MFE mode

[Li, Zhong]

> From: libav-devel [mailto:libav-devel-boun...@libav.org] On Behalf Of
> Diego Biurrun
> Sent: Tuesday, May 22, 2018 6:30 PM
> To: libav development 
> Subject: Re: [libav-devel] [PATCH V2] lavc/qsvenc: add an option to disable
> MFE mode
> 
> On Tue, May 22, 2018 at 08:03:00AM +, Li, Zhong wrote:
> > > -Original Message-
> > > From: libav-devel [mailto:libav-devel-boun...@libav.org] On Behalf
> > > Of Diego Biurrun
> > > Sent: Monday, May 21, 2018 11:17 PM
> > > To: libav development 
> > > Subject: Re: [libav-devel] [PATCH V2] lavc/qsvenc: add an option to
> > > disable MFE mode
> > >
> > > On Mon, May 21, 2018 at 02:33:28PM +0800, Zhong Li wrote:
> > > >
> > > > V2: remove the manual option since it is not supported now.
> > >
> > > This looks like a patch annotation that should not be part of the
> > > log message.
> >
> > MFE manual mode hasn't been implemented in libav right now, so the
> option shouldn't been exposed. I am not sure where the better place is to
> give such an annotation.
> > I am ok to send an updated patch to remove it if without any other
> changes required. Or anyone can help to modify the log message when
> merge this patch?
> 
> Use the --annotate option to git-send-email and add the annotation below
> the "---".

Thanks for your explanation and I understand it now.
But I prefer to keep it in log message because I want to explain why MSDK has 
MFX_MF_MANUAL but we don't expose it.
And it also can remind developer if he want to add such an option, he need to 
change current MFE implantation.
If the log message is not very clear, I can update it, but I don't think take 
it as an annotation is a good idea since it will be lost when patch applied. 
How do you think?

> 
> Diego
> ___
> libav-devel mailing list
> libav-devel@libav.org
> https://lists.libav.org/mailman/listinfo/libav-devel
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH V2] lavc/qsvenc: add an option to disable MFE mode

[Maxym Dmytrychenko]

thanks Luca,

this patch should be reasonable

On Mon, May 21, 2018 at 9:58 AM, Luca Barbato  wrote:

> On 21/05/2018 08:33, Zhong Li wrote:
> > Not convenient if using numerals to set MFE mode. It is ambiguous
> > and misleading (e.g: user may misunderstand setting mfmode to 1 is to
> > enable MFE but actually it is to disable MFE, and set it to be 5 or
> above is meaningless).
> >
> > V2: remove the manual option since it is not supported now.
> >
> > Signed-off-by: Zhong Li 
> > ---
> >  libavcodec/qsvenc_h264.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/libavcodec/qsvenc_h264.c b/libavcodec/qsvenc_h264.c
> > index ae00ff8..2ecdb10 100644
> > --- a/libavcodec/qsvenc_h264.c
> > +++ b/libavcodec/qsvenc_h264.c
> > @@ -94,7 +94,9 @@ static const AVOption options[] = {
> >  { "aud", "Insert the Access Unit Delimiter NAL", OFFSET(qsv.aud),
> AV_OPT_TYPE_INT, { .i64 = 0 }, 0, 1, VE},
> >
> >  #if QSV_HAVE_MF
> > -{ "mfmode", "Multi-Frame Mode", OFFSET(qsv.mfmode),
> AV_OPT_TYPE_INT, { .i64 = MFX_MF_AUTO }, 0, INT_MAX, VE },
> > +{ "mfmode", "Multi-Frame Mode", OFFSET(qsv.mfmode),
> AV_OPT_TYPE_INT, { .i64 = MFX_MF_AUTO }, MFX_MF_DEFAULT, MFX_MF_AUTO, VE,
> "mfmode"},
> > +{ "off", NULL, 0, AV_OPT_TYPE_CONST, { .i64 = MFX_MF_DISABLED
> }, INT_MIN, INT_MAX, VE, "mfmode" },
> > +{ "auto"   , NULL, 0, AV_OPT_TYPE_CONST, { .i64 = MFX_MF_AUTO
>  }, INT_MIN, INT_MAX, VE, "mfmode" },
> >  #endif
> >
> >  { NULL },
> >
>
> Sounds fine to me, the previous iteration was on hold since Maxym wanted
> to test it.
>
> I guess this time it should work as intended on every current mfx release
> :)
>
> lu
> ___
> libav-devel mailing list
> libav-devel@libav.org
> https://lists.libav.org/mailman/listinfo/libav-devel
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH V2] lavc/qsvenc: add an option to disable MFE mode

[Diego Biurrun]

On Tue, May 22, 2018 at 08:03:00AM +, Li, Zhong wrote:
> > -Original Message-
> > From: libav-devel [mailto:libav-devel-boun...@libav.org] On Behalf Of
> > Diego Biurrun
> > Sent: Monday, May 21, 2018 11:17 PM
> > To: libav development 
> > Subject: Re: [libav-devel] [PATCH V2] lavc/qsvenc: add an option to disable
> > MFE mode
> > 
> > On Mon, May 21, 2018 at 02:33:28PM +0800, Zhong Li wrote:
> > >
> > > V2: remove the manual option since it is not supported now.
> > 
> > This looks like a patch annotation that should not be part of the log
> > message.
> 
> MFE manual mode hasn't been implemented in libav right now, so the option 
> shouldn't been exposed. I am not sure where the better place is to give such 
> an annotation.
> I am ok to send an updated patch to remove it if without any other changes 
> required. Or anyone can help to modify the log message when merge this patch?

Use the --annotate option to git-send-email and add the annotation below the 
"---".

Diego
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH V2] lavc/qsvenc: add an option to disable MFE mode

[Li, Zhong]

> -Original Message-
> From: libav-devel [mailto:libav-devel-boun...@libav.org] On Behalf Of
> Diego Biurrun
> Sent: Monday, May 21, 2018 11:17 PM
> To: libav development 
> Subject: Re: [libav-devel] [PATCH V2] lavc/qsvenc: add an option to disable
> MFE mode
> 
> On Mon, May 21, 2018 at 02:33:28PM +0800, Zhong Li wrote:
> >
> > V2: remove the manual option since it is not supported now.
> 
> This looks like a patch annotation that should not be part of the log
> message.

MFE manual mode hasn't been implemented in libav right now, so the option 
shouldn't been exposed. I am not sure where the better place is to give such an 
annotation.
I am ok to send an updated patch to remove it if without any other changes 
required. Or anyone can help to modify the log message when merge this patch?

> 
> Diego

___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel