[V2] lavc/qsvenc: add an option to disable MFE mode

Message ID 1526884408-13292-1-git-send-email-zhong.li@intel.com
State New
Headers show
Series
  • [V2] lavc/qsvenc: add an option to disable MFE mode
Related show

Commit Message

Li, Zhong May 21, 2018, 6:33 a.m.
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 <zhong.li@intel.com>
---
 libavcodec/qsvenc_h264.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Luca Barbato May 21, 2018, 7:58 a.m. | #1
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 <zhong.li@intel.com>
> ---
>  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
Diego Biurrun May 21, 2018, 3:16 p.m. | #2
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.

Diego
Li, Zhong May 22, 2018, 8:03 a.m. | #3
> -----Original Message-----
> From: libav-devel [mailto:libav-devel-bounces@libav.org] On Behalf Of
> Diego Biurrun
> Sent: Monday, May 21, 2018 11:17 PM
> To: libav development <libav-devel@libav.org>
> 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
Diego Biurrun May 22, 2018, 10:29 a.m. | #4
On Tue, May 22, 2018 at 08:03:00AM +0000, Li, Zhong wrote:
> > -----Original Message-----
> > From: libav-devel [mailto:libav-devel-bounces@libav.org] On Behalf Of
> > Diego Biurrun
> > Sent: Monday, May 21, 2018 11:17 PM
> > To: libav development <libav-devel@libav.org>
> > 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
Maxym Dmytrychenko May 22, 2018, 12:52 p.m. | #5
thanks Luca,

this patch should be reasonable

On Mon, May 21, 2018 at 9:58 AM, Luca Barbato <lu_zero@gentoo.org> 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 <zhong.li@intel.com>
> > ---
> >  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
Li, Zhong May 23, 2018, 3:15 a.m. | #6
> From: libav-devel [mailto:libav-devel-bounces@libav.org] On Behalf Of
> Diego Biurrun
> Sent: Tuesday, May 22, 2018 6:30 PM
> To: libav development <libav-devel@libav.org>
> Subject: Re: [libav-devel] [PATCH V2] lavc/qsvenc: add an option to disable
> MFE mode
> 
> On Tue, May 22, 2018 at 08:03:00AM +0000, Li, Zhong wrote:
> > > -----Original Message-----
> > > From: libav-devel [mailto:libav-devel-bounces@libav.org] On Behalf
> > > Of Diego Biurrun
> > > Sent: Monday, May 21, 2018 11:17 PM
> > > To: libav development <libav-devel@libav.org>
> > > 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
Li, Zhong May 23, 2018, 3:16 a.m. | #7
Thanks for Mayxm/Luca/Diego's review. : ) 

> -----Original Message-----
> From: libav-devel [mailto:libav-devel-bounces@libav.org] On Behalf Of
> Maxym Dmytrychenko
> Sent: Tuesday, May 22, 2018 8:53 PM
> To: libav development <libav-devel@libav.org>
> 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 <lu_zero@gentoo.org>
> 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 <zhong.li@intel.com>
> > > ---
> > >  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
Diego Biurrun May 23, 2018, 8:13 a.m. | #8
On Wed, May 23, 2018 at 03:15:14AM +0000, Li, Zhong wrote:
> > From: libav-devel [mailto:libav-devel-bounces@libav.org] On Behalf Of
> > Diego Biurrun
> > Sent: Tuesday, May 22, 2018 6:30 PM
> > To: libav development <libav-devel@libav.org>
> > Subject: Re: [libav-devel] [PATCH V2] lavc/qsvenc: add an option to disable
> > MFE mode
> > 
> > On Tue, May 22, 2018 at 08:03:00AM +0000, Li, Zhong wrote:
> > > > -----Original Message-----
> > > > From: libav-devel [mailto:libav-devel-bounces@libav.org] On Behalf
> > > > Of Diego Biurrun
> > > > Sent: Monday, May 21, 2018 11:17 PM
> > > > To: libav development <libav-devel@libav.org>
> > > > 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?

As part of the log message it does not make any sense as currently written.
The patch does not remove the manual option. If you feel it is valuable to
add something like the text above into the log message, go right ahead.

Diego
Li, Zhong May 23, 2018, 10:04 a.m. | #9
> From: libav-devel [mailto:libav-devel-bounces@libav.org] On Behalf Of
> Diego Biurrun
> Sent: Wednesday, May 23, 2018 4:13 PM
> To: libav development <libav-devel@libav.org>
> Subject: Re: [libav-devel] [PATCH V2] lavc/qsvenc: add an option to disable
> MFE mode
> 
> On Wed, May 23, 2018 at 03:15:14AM +0000, Li, Zhong wrote:
> > > From: libav-devel [mailto:libav-devel-bounces@libav.org] On Behalf
> > > Of Diego Biurrun
> > > Sent: Tuesday, May 22, 2018 6:30 PM
> > > To: libav development <libav-devel@libav.org>
> > > Subject: Re: [libav-devel] [PATCH V2] lavc/qsvenc: add an option to
> > > disable MFE mode
> > >
> > > On Tue, May 22, 2018 at 08:03:00AM +0000, Li, Zhong wrote:
> > > > > -----Original Message-----
> > > > > From: libav-devel [mailto:libav-devel-bounces@libav.org] On
> > > > > Behalf Of Diego Biurrun
> > > > > Sent: Monday, May 21, 2018 11:17 PM
> > > > > To: libav development <libav-devel@libav.org>
> > > > > 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?
> 
> As part of the log message it does not make any sense as currently written.
> The patch does not remove the manual option. If you feel it is valuable to
> add something like the text above into the log message, go right ahead.

Thanks for your suggestion. I should get your point and I've sent a new one to update the log message. 

> 
> Diego

Patch

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 },