Discussion:
[libav-devel] [PATCH V2] lavc/qsvenc: add an option to disable MFE mode
Luca Barbato
2018-05-21 07:58:25 UTC
Permalink
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.
---
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
2018-05-21 15:16:35 UTC
Permalink
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
2018-05-22 08:03:00 UTC
Permalink
-----Original Message-----
Diego Biurrun
Sent: Monday, May 21, 2018 11:17 PM
Subject: Re: [libav-devel] [PATCH V2] lavc/qsvenc: add an option to disable
MFE mode
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
2018-05-22 10:29:34 UTC
Permalink
Post by Li, Zhong
-----Original Message-----
Diego Biurrun
Sent: Monday, May 21, 2018 11:17 PM
Subject: Re: [libav-devel] [PATCH V2] lavc/qsvenc: add an option to disable
MFE mode
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
Li, Zhong
2018-05-23 03:15:14 UTC
Permalink
Diego Biurrun
Sent: Tuesday, May 22, 2018 6:30 PM
Subject: Re: [libav-devel] [PATCH V2] lavc/qsvenc: add an option to disable
MFE mode
Post by Li, Zhong
-----Original Message-----
Of Diego Biurrun
Sent: Monday, May 21, 2018 11:17 PM
Subject: Re: [libav-devel] [PATCH V2] lavc/qsvenc: add an option to
disable MFE mode
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.
Post by Li, Zhong
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
https://lists.libav.org/mailman/listinfo/libav-devel
Diego Biurrun
2018-05-23 08:13:15 UTC
Permalink
Post by Li, Zhong
Diego Biurrun
Sent: Tuesday, May 22, 2018 6:30 PM
Subject: Re: [libav-devel] [PATCH V2] lavc/qsvenc: add an option to disable
MFE mode
Post by Li, Zhong
-----Original Message-----
Of Diego Biurrun
Sent: Monday, May 21, 2018 11:17 PM
Subject: Re: [libav-devel] [PATCH V2] lavc/qsvenc: add an option to
disable MFE mode
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.
Post by Li, Zhong
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
2018-05-23 10:04:26 UTC
Permalink
Diego Biurrun
Sent: Wednesday, May 23, 2018 4:13 PM
Subject: Re: [libav-devel] [PATCH V2] lavc/qsvenc: add an option to disable
MFE mode
Post by Li, Zhong
Of Diego Biurrun
Sent: Tuesday, May 22, 2018 6:30 PM
Subject: Re: [libav-devel] [PATCH V2] lavc/qsvenc: add an option to
disable MFE mode
Post by Li, Zhong
-----Original Message-----
Behalf Of Diego Biurrun
Sent: Monday, May 21, 2018 11:17 PM
Subject: Re: [libav-devel] [PATCH V2] lavc/qsvenc: add an option
to disable MFE mode
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.
Post by Li, Zhong
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.
Post by Li, Zhong
And it also can remind developer if he want to add such an option, he need
to change current MFE implantation.
Post by Li, Zhong
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.
Post by Li, Zhong
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
Maxym Dmytrychenko
2018-05-22 12:52:47 UTC
Permalink
thanks Luca,

this patch should be reasonable
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.
---
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
https://lists.libav.org/mailman/listinfo/libav-devel
Li, Zhong
2018-05-23 03:16:12 UTC
Permalink
Thanks for Mayxm/Luca/Diego's review. : )
-----Original Message-----
Maxym Dmytrychenko
Sent: Tuesday, May 22, 2018 8:53 PM
Subject: Re: [libav-devel] [PATCH V2] lavc/qsvenc: add an option to disable
MFE mode
thanks Luca,
this patch should be reasonable
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.
---
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
https://lists.libav.org/mailman/listinfo/libav-devel
_______________________________________________
libav-devel mailing list
https://lists.libav.org/mailman/listinfo/libav-devel
Loading...