Re: [FFmpeg-devel] [PATCH v4 1/9] lavc/libopenh264enc: Add default qmin/qmax support

2020-04-28 Thread Martin Storsjö 

On Tue, 28 Apr 2020, Fu, Linjie wrote:


From: ffmpeg-devel  On Behalf Of
Martin Storsjö
Sent: Tuesday, April 28, 2020 14:31
To: Fu, Linjie 
Cc: FFmpeg development discussions and patches 
Subject: Re: [FFmpeg-devel] [PATCH v4 1/9] lavc/libopenh264enc: Add
default qmin/qmax support

On Tue, 28 Apr 2020, Fu, Linjie wrote:


From: Martin Storsjö 
Sent: Tuesday, April 28, 2020 14:19
To: Fu, Linjie 
Cc: FFmpeg development discussions and patches 
Subject: RE: [FFmpeg-devel] [PATCH v4 1/9] lavc/libopenh264enc: Add
default qmin/qmax support

On Tue, 28 Apr 2020, Fu, Linjie wrote:


From: Martin Storsjö 
Sent: Tuesday, April 28, 2020 03:27

If qmax/qmin < 0, i.e. wasn't specified by the user, wouldn't it be

better

to not touch param.iMax/MinQp at all (and use the default value of the
library, which may change between versions), instead of overriding it

with

a value hardcoded here?


Okay, this seems more natural if the recommended QP range varies

between

versions, though one of my original purposes is to avoid the warning in

default

situation for changing the QP inside libopenh264 library.


Well in general I'd want to avoid hardcoding opinionated defaults within
our own wrapper - I'd like it to behave as close to what upstream
intended, so that whatever issues we see with defaults, are the same
issues that everyone else sees as well, so any fixes to those defaults
upstream also end up for us - so we don't get stuck on whatever we

thought

was a good default at some point.


Agree, this makes more sense.


What warnings about changing QP are you referring to?

Warning:Change QP Range from(0,51) to (12,42)


https://github.com/cisco/openh264/blob/master/codec/encoder/core/src/e
ncoder_ext.cpp#L375


The main reason is that "0" is not supported well, which is the default
value of iMinQp.


Ah, right.

In that case, setting some other default might make sense indeed.
How/where does OpenH264 set suitable values for this, in order not to get
the warnings everywhere? Are the values e.g. 12-42 hardcoded everywhere
in
every caller?


IIRC, it depends on the iUsageType, and hardcoded in the libopenh264 library 
function
ParamValidation() in [1]:
For SCREEN_CONTENT_REAL_TIME,
it's (MIN_SCREEN_QP, MAX_SCREEN_QP), e.g. (26, 35);
For other usage like CAMERA_VIDEO_REAL_TIME by default,
It's (GOM_MIN_QP_MODE, MAX_LOW_BR_QP), e.g. (12, 42)

- Linjie

[1] 
https://github.com/cisco/openh264/blob/master/codec/encoder/core/src/encoder_ext.cpp#L379


Right, so looking at that code, it looks like it has a good intent - if 
the user hasn't specified that he wants a custom setting of min/max qp, 
then it uses the defaults.


So the problem is that this writes a warning in the log - and you want to 
avoid the warning.


Wouldn't it just be better to change this message to WELS_LOG_INFO within 
openh264, to avoid it being treated as a warning - as it's not a fault, 
it's the intended behaviour of picking sensible defaults when the user 
hasn't requested anything in particular?


// Martin
___
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".

Re: [FFmpeg-devel] [PATCH v4 1/9] lavc/libopenh264enc: Add default qmin/qmax support

2020-04-28 Thread Fu, Linjie 
> From: ffmpeg-devel  On Behalf Of
> Martin Storsjö
> Sent: Tuesday, April 28, 2020 14:31
> To: Fu, Linjie 
> Cc: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH v4 1/9] lavc/libopenh264enc: Add
> default qmin/qmax support
> 
> On Tue, 28 Apr 2020, Fu, Linjie wrote:
> 
> >> From: Martin Storsjö 
> >> Sent: Tuesday, April 28, 2020 14:19
> >> To: Fu, Linjie 
> >> Cc: FFmpeg development discussions and patches  >> de...@ffmpeg.org>
> >> Subject: RE: [FFmpeg-devel] [PATCH v4 1/9] lavc/libopenh264enc: Add
> >> default qmin/qmax support
> >>
> >> On Tue, 28 Apr 2020, Fu, Linjie wrote:
> >>
> >>>> From: Martin Storsjö 
> >>>> Sent: Tuesday, April 28, 2020 03:27
> >>>>
> >>>> If qmax/qmin < 0, i.e. wasn't specified by the user, wouldn't it be
> better
> >>>> to not touch param.iMax/MinQp at all (and use the default value of the
> >>>> library, which may change between versions), instead of overriding it
> with
> >>>> a value hardcoded here?
> >>>>
> >>> Okay, this seems more natural if the recommended QP range varies
> >> between
> >>> versions, though one of my original purposes is to avoid the warning in
> >> default
> >>> situation for changing the QP inside libopenh264 library.
> >>
> >> Well in general I'd want to avoid hardcoding opinionated defaults within
> >> our own wrapper - I'd like it to behave as close to what upstream
> >> intended, so that whatever issues we see with defaults, are the same
> >> issues that everyone else sees as well, so any fixes to those defaults
> >> upstream also end up for us - so we don't get stuck on whatever we
> thought
> >> was a good default at some point.
> >
> > Agree, this makes more sense.
> >
> >> What warnings about changing QP are you referring to?
> > Warning:Change QP Range from(0,51) to (12,42)
> >
> https://github.com/cisco/openh264/blob/master/codec/encoder/core/src/e
> ncoder_ext.cpp#L375
> >
> > The main reason is that "0" is not supported well, which is the default
> > value of iMinQp.
> 
> Ah, right.
> 
> In that case, setting some other default might make sense indeed.
> How/where does OpenH264 set suitable values for this, in order not to get
> the warnings everywhere? Are the values e.g. 12-42 hardcoded everywhere
> in
> every caller?
> 
IIRC, it depends on the iUsageType, and hardcoded in the libopenh264 library 
function
ParamValidation() in [1]:
For SCREEN_CONTENT_REAL_TIME,
it's (MIN_SCREEN_QP, MAX_SCREEN_QP), e.g. (26, 35);
For other usage like CAMERA_VIDEO_REAL_TIME by default,
It's (GOM_MIN_QP_MODE, MAX_LOW_BR_QP), e.g. (12, 42)

- Linjie

[1] 
https://github.com/cisco/openh264/blob/master/codec/encoder/core/src/encoder_ext.cpp#L379
___
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".

Re: [FFmpeg-devel] [PATCH v4 1/9] lavc/libopenh264enc: Add default qmin/qmax support

2020-04-28 Thread Martin Storsjö 

On Tue, 28 Apr 2020, Fu, Linjie wrote:


From: Martin Storsjö 
Sent: Tuesday, April 28, 2020 14:19
To: Fu, Linjie 
Cc: FFmpeg development discussions and patches 
Subject: RE: [FFmpeg-devel] [PATCH v4 1/9] lavc/libopenh264enc: Add
default qmin/qmax support

On Tue, 28 Apr 2020, Fu, Linjie wrote:


From: Martin Storsjö 
Sent: Tuesday, April 28, 2020 03:27

If qmax/qmin < 0, i.e. wasn't specified by the user, wouldn't it be better
to not touch param.iMax/MinQp at all (and use the default value of the
library, which may change between versions), instead of overriding it with
a value hardcoded here?


Okay, this seems more natural if the recommended QP range varies

between

versions, though one of my original purposes is to avoid the warning in

default

situation for changing the QP inside libopenh264 library.


Well in general I'd want to avoid hardcoding opinionated defaults within
our own wrapper - I'd like it to behave as close to what upstream
intended, so that whatever issues we see with defaults, are the same
issues that everyone else sees as well, so any fixes to those defaults
upstream also end up for us - so we don't get stuck on whatever we thought
was a good default at some point.


Agree, this makes more sense.


What warnings about changing QP are you referring to?

Warning:Change QP Range from(0,51) to (12,42)
https://github.com/cisco/openh264/blob/master/codec/encoder/core/src/encoder_ext.cpp#L375

The main reason is that "0" is not supported well, which is the default 
value of iMinQp.


Ah, right.

In that case, setting some other default might make sense indeed. 
How/where does OpenH264 set suitable values for this, in order not to get 
the warnings everywhere? Are the values e.g. 12-42 hardcoded everywhere in 
every caller?


// Martin
___
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".

Re: [FFmpeg-devel] [PATCH v4 1/9] lavc/libopenh264enc: Add default qmin/qmax support

2020-04-28 Thread Fu, Linjie 
> From: Martin Storsjö 
> Sent: Tuesday, April 28, 2020 14:19
> To: Fu, Linjie 
> Cc: FFmpeg development discussions and patches  de...@ffmpeg.org>
> Subject: RE: [FFmpeg-devel] [PATCH v4 1/9] lavc/libopenh264enc: Add
> default qmin/qmax support
> 
> On Tue, 28 Apr 2020, Fu, Linjie wrote:
> 
> >> From: Martin Storsjö 
> >> Sent: Tuesday, April 28, 2020 03:27
> >>
> >> If qmax/qmin < 0, i.e. wasn't specified by the user, wouldn't it be better
> >> to not touch param.iMax/MinQp at all (and use the default value of the
> >> library, which may change between versions), instead of overriding it with
> >> a value hardcoded here?
> >>
> > Okay, this seems more natural if the recommended QP range varies
> between
> > versions, though one of my original purposes is to avoid the warning in
> default
> > situation for changing the QP inside libopenh264 library.
> 
> Well in general I'd want to avoid hardcoding opinionated defaults within
> our own wrapper - I'd like it to behave as close to what upstream
> intended, so that whatever issues we see with defaults, are the same
> issues that everyone else sees as well, so any fixes to those defaults
> upstream also end up for us - so we don't get stuck on whatever we thought
> was a good default at some point.

Agree, this makes more sense.
 
> What warnings about changing QP are you referring to?
Warning:Change QP Range from(0,51) to (12,42) 
https://github.com/cisco/openh264/blob/master/codec/encoder/core/src/encoder_ext.cpp#L375
 

The main reason is that "0" is not supported well, which is the default value 
of iMinQp.

- Linjie
___
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".

Re: [FFmpeg-devel] [PATCH v4 1/9] lavc/libopenh264enc: Add default qmin/qmax support

2020-04-28 Thread Martin Storsjö 

On Tue, 28 Apr 2020, Fu, Linjie wrote:


From: Martin Storsjö 
Sent: Tuesday, April 28, 2020 03:27

If qmax/qmin < 0, i.e. wasn't specified by the user, wouldn't it be better
to not touch param.iMax/MinQp at all (and use the default value of the
library, which may change between versions), instead of overriding it with
a value hardcoded here?


Okay, this seems more natural if the recommended QP range varies between
versions, though one of my original purposes is to avoid the warning in default
situation for changing the QP inside libopenh264 library.


Well in general I'd want to avoid hardcoding opinionated defaults within 
our own wrapper - I'd like it to behave as close to what upstream 
intended, so that whatever issues we see with defaults, are the same 
issues that everyone else sees as well, so any fixes to those defaults 
upstream also end up for us - so we don't get stuck on whatever we thought 
was a good default at some point.


What warnings about changing QP are you referring to?

// Martin
___
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".

Re: [FFmpeg-devel] [PATCH v4 1/9] lavc/libopenh264enc: Add default qmin/qmax support

2020-04-27 Thread Fu, Linjie 
> From: Martin Storsjö 
> Sent: Tuesday, April 28, 2020 03:27
> 
> If qmax/qmin < 0, i.e. wasn't specified by the user, wouldn't it be better
> to not touch param.iMax/MinQp at all (and use the default value of the
> library, which may change between versions), instead of overriding it with
> a value hardcoded here?
> 
Okay, this seems more natural if the recommended QP range varies between
versions, though one of my original purposes is to avoid the warning in default
situation for changing the QP inside libopenh264 library.

Updated locally as suggested, and thanks for the review for the whole set.

- Linjie
___
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".

Re: [FFmpeg-devel] [PATCH v4 1/9] lavc/libopenh264enc: Add default qmin/qmax support

2020-04-27 Thread Martin Storsjö 

On Wed, 15 Apr 2020, Linjie Fu wrote:


Set default QP range to (1, 51) instead of (2, 32).

QP = 0 is not well supported currently in libopenh264. If iMaxQp/iMinQp
equals 0, the QP range would be changed unexpectedly inside libopenh264
with a warning:

Warning:Change QP Range from(0,51) to (12,42)

[1] 

[2] 

Signed-off-by: Linjie Fu 
---
libavcodec/libopenh264enc.c | 10 ++
1 file changed, 10 insertions(+)

diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
index dd5d4ee..c7ae5b1 100644
--- a/libavcodec/libopenh264enc.c
+++ b/libavcodec/libopenh264enc.c
@@ -135,6 +135,9 @@ FF_ENABLE_DEPRECATION_WARNINGS
param.iTargetBitrate = avctx->bit_rate;
param.iMaxBitrate= FFMAX(avctx->rc_max_rate, 
avctx->bit_rate);
param.iRCMode= RC_QUALITY_MODE;
+// QP = 0 is not well supported, so default to (1, 51)
+param.iMaxQp = avctx->qmax >= 0 ? av_clip(avctx->qmax, 
1, 51) : 51;
+param.iMinQp = avctx->qmin >= 0 ? av_clip(avctx->qmin, 
1, param.iMaxQp) : 1;


If qmax/qmin < 0, i.e. wasn't specified by the user, wouldn't it be better 
to not touch param.iMax/MinQp at all (and use the default value of the 
library, which may change between versions), instead of overriding it with 
a value hardcoded here?


// Martin

___
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] [PATCH v4 1/9] lavc/libopenh264enc: Add default qmin/qmax support

2020-04-14 Thread Linjie Fu 
Set default QP range to (1, 51) instead of (2, 32).

QP = 0 is not well supported currently in libopenh264. If iMaxQp/iMinQp
equals 0, the QP range would be changed unexpectedly inside libopenh264
with a warning:

Warning:Change QP Range from(0,51) to (12,42)

[1] 

[2] 

Signed-off-by: Linjie Fu 
---
 libavcodec/libopenh264enc.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
index dd5d4ee..c7ae5b1 100644
--- a/libavcodec/libopenh264enc.c
+++ b/libavcodec/libopenh264enc.c
@@ -135,6 +135,9 @@ FF_ENABLE_DEPRECATION_WARNINGS
 param.iTargetBitrate = avctx->bit_rate;
 param.iMaxBitrate= FFMAX(avctx->rc_max_rate, 
avctx->bit_rate);
 param.iRCMode= RC_QUALITY_MODE;
+// QP = 0 is not well supported, so default to (1, 51)
+param.iMaxQp = avctx->qmax >= 0 ? av_clip(avctx->qmax, 
1, 51) : 51;
+param.iMinQp = avctx->qmin >= 0 ? av_clip(avctx->qmin, 
1, param.iMaxQp) : 1;
 param.iTemporalLayerNum  = 1;
 param.iSpatialLayerNum   = 1;
 param.bEnableDenoise = 0;
@@ -331,6 +334,12 @@ static int svc_encode_frame(AVCodecContext *avctx, 
AVPacket *avpkt,
 return 0;
 }
 
+static const AVCodecDefault svc_enc_defaults[] = {
+{ "qmin",  "-1"},
+{ "qmax",  "-1"},
+{ NULL },
+};
+
 AVCodec ff_libopenh264_encoder = {
 .name   = "libopenh264",
 .long_name  = NULL_IF_CONFIG_SMALL("OpenH264 H.264 / AVC / MPEG-4 AVC 
/ MPEG-4 part 10"),
@@ -344,6 +353,7 @@ AVCodec ff_libopenh264_encoder = {
 .caps_internal  = FF_CODEC_CAP_INIT_THREADSAFE | FF_CODEC_CAP_INIT_CLEANUP,
 .pix_fmts   = (const enum AVPixelFormat[]){ AV_PIX_FMT_YUV420P,
 AV_PIX_FMT_NONE },
+.defaults   = svc_enc_defaults,
 .priv_class = ,
 .wrapper_name   = "libopenh264",
 };
-- 
2.7.4

___
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".