| Message ID | 20190828211843.50548-1-elliottk@google.com |
|---|---|
| State | Superseded |
| Headers | show |
Am Mi., 28. Aug. 2019 um 23:26 Uhr schrieb Elliott Karpilovsky <elliottk-at-google.com@ffmpeg.org>: > > Current default is 200kbps, which produces inconsistent > results (too high for low-res, too low for hi-res). Use > CRF instead, which will adapt. Affects vp8/vp9. Also > have VP8 use a default bitrate of 256kbps. Would it simplify the patch if -crf 0 would indicate lossless encoding? Carl Eugen
On Wed, Aug 28, 2019 at 3:11 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > > Am Mi., 28. Aug. 2019 um 23:26 Uhr schrieb Elliott Karpilovsky > <elliottk-at-google.com@ffmpeg.org>: > > > > Current default is 200kbps, which produces inconsistent > > results (too high for low-res, too low for hi-res). Use > > CRF instead, which will adapt. Affects vp8/vp9. Also > > have VP8 use a default bitrate of 256kbps. > > Would it simplify the patch if -crf 0 would indicate lossless encoding? > > Carl Eugen I do not believe so. I believe that there would be more code changes to switch the checks from ctx->lossless to ctx->crf, and the code might need to be changed to no longer accept -lossless as a flag (unless ffmpeg is okay with there being two ways to trigger it).
Am Do., 29. Aug. 2019 um 18:12 Uhr schrieb Elliott Karpilovsky <elliottk-at-google.com@ffmpeg.org>: > > On Wed, Aug 28, 2019 at 3:11 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > > > > Am Mi., 28. Aug. 2019 um 23:26 Uhr schrieb Elliott Karpilovsky > > <elliottk-at-google.com@ffmpeg.org>: > > > > > > Current default is 200kbps, which produces inconsistent > > > results (too high for low-res, too low for hi-res). Use > > > CRF instead, which will adapt. Affects vp8/vp9. Also > > > have VP8 use a default bitrate of 256kbps. > > > > Would it simplify the patch if -crf 0 would indicate lossless encoding? > I do not believe so. I believe that there would be more code changes > to switch the checks from ctx->lossless to ctx->crf, Of course but that wasn't my question. I believe it is a horrible bug that x264 uses crf 0 for lossless encoding, while libvpx uses another option. If fixing this bug simplifies your patch, I believe it is worth the effort. And the pain with "lossless" is so big imo that changing it cannot hurt. Carl Eugen
On Thu, Aug 29, 2019 at 7:17 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > > Am Do., 29. Aug. 2019 um 18:12 Uhr schrieb Elliott Karpilovsky > <elliottk-at-google.com@ffmpeg.org>: > > > > On Wed, Aug 28, 2019 at 3:11 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > > > > > > Am Mi., 28. Aug. 2019 um 23:26 Uhr schrieb Elliott Karpilovsky > > > <elliottk-at-google.com@ffmpeg.org>: > > > > > > > > Current default is 200kbps, which produces inconsistent > > > > results (too high for low-res, too low for hi-res). Use > > > > CRF instead, which will adapt. Affects vp8/vp9. Also > > > > have VP8 use a default bitrate of 256kbps. > > > > > > Would it simplify the patch if -crf 0 would indicate lossless encoding? > > > I do not believe so. I believe that there would be more code changes > > to switch the checks from ctx->lossless to ctx->crf, > > Of course but that wasn't my question. > I believe it is a horrible bug that x264 uses crf 0 for lossless encoding, > while libvpx uses another option. If fixing this bug simplifies your > patch, I believe it is worth the effort. > Just for the record, libx264 only does lossless for CRF=0 for 8bit coding. The quantizer range for CRF was expanded downwards for higher bit depths since the results were similar for lossy coding. In that case you either have to poke the right negative value, or set quantizer to zero - which is the option that works for all bit depths with libx264. That said, I do not have heavy objections one way or another regarding how the libvpx wrapper defines CRF (as I think the definition for VP8 and VP9 is already in general different from what libx264 does - like bit rate being required for VP8 etc). Just wanted to refresh people's minds regarding libx264's CRF behavior with lossless coding. Best regards, Jan
On Thu, Aug 29, 2019 at 10:14 AM Jan Ekström <jeebjp@gmail.com> wrote: > > On Thu, Aug 29, 2019 at 7:17 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > > > > Am Do., 29. Aug. 2019 um 18:12 Uhr schrieb Elliott Karpilovsky > > <elliottk-at-google.com@ffmpeg.org>: > > > > > > On Wed, Aug 28, 2019 at 3:11 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > > > > > > > > Am Mi., 28. Aug. 2019 um 23:26 Uhr schrieb Elliott Karpilovsky > > > > <elliottk-at-google.com@ffmpeg.org>: > > > > > > > > > > Current default is 200kbps, which produces inconsistent > > > > > results (too high for low-res, too low for hi-res). Use > > > > > CRF instead, which will adapt. Affects vp8/vp9. Also > > > > > have VP8 use a default bitrate of 256kbps. > > > > > > > > Would it simplify the patch if -crf 0 would indicate lossless encoding? > > > > > I do not believe so. I believe that there would be more code changes > > > to switch the checks from ctx->lossless to ctx->crf, > > > > Of course but that wasn't my question. > > I believe it is a horrible bug that x264 uses crf 0 for lossless encoding, > > while libvpx uses another option. If fixing this bug simplifies your > > patch, I believe it is worth the effort. > > > > Just for the record, libx264 only does lossless for CRF=0 for 8bit > coding. The quantizer range for CRF was expanded downwards for higher > bit depths since the results were similar for lossy coding. In that > case you either have to poke the right negative value, or set > quantizer to zero - which is the option that works for all bit depths > with libx264. > > That said, I do not have heavy objections one way or another regarding > how the libvpx wrapper defines CRF (as I think the definition for VP8 > and VP9 is already in general different from what libx264 does - like > bit rate being required for VP8 etc). Just wanted to refresh people's > minds regarding libx264's CRF behavior with lossless coding. > > Best regards, > Jan Thanks for the context. My preference is to keep this patch about changing the default behavior. I would not oppose someone else writing another patch to change CRF=0 to lossless, but it feels like these two should be separate.
Hi, On Wed, Aug 28, 2019 at 2:26 PM Elliott Karpilovsky <elliottk-at-google.com@ffmpeg.org> wrote: > > Current default is 200kbps, which produces inconsistent > results (too high for low-res, too low for hi-res). Use > CRF instead, which will adapt. Affects vp8/vp9. Also > have VP8 use a default bitrate of 256kbps. > --- > libavcodec/libvpxenc.c | 71 +++++++++++++++++++++++++++++++++++------- > libavcodec/version.h | 2 +- > 2 files changed, 61 insertions(+), 12 deletions(-) > Some cosmetics, seems to work as expected with tip of tree and v1.4.0. > diff --git a/libavcodec/libvpxenc.c b/libavcodec/libvpxenc.c > index feb52ea0dd..42504ab95e 100644 > --- a/libavcodec/libvpxenc.c > +++ b/libavcodec/libvpxenc.c > @@ -510,6 +510,63 @@ static void set_color_range(AVCodecContext *avctx) > #endif > #endif > > +/** > + * Set the target bitrate to VPX library default. Also set CRF to 32 if needed. > + */ > +static void set_vp8_defaults(AVCodecContext *avctx, > + struct vpx_codec_enc_cfg *enccfg) { Indent should be 4 and the brace for a function on a new line [1]. > [...] > + > +/** > + * Called when the bitrate is not set. It sets appropriate default values for > + * bit-rate and CRF. 'bitrate' is more common in this file. > [...] > diff --git a/libavcodec/version.h b/libavcodec/version.h > index e70ebc0c70..cda6dbae47 100644 > --- a/libavcodec/version.h > +++ b/libavcodec/version.h > @@ -29,7 +29,7 @@ > > #define LIBAVCODEC_VERSION_MAJOR 58 > #define LIBAVCODEC_VERSION_MINOR 55 > -#define LIBAVCODEC_VERSION_MICRO 101 > +#define LIBAVCODEC_VERSION_MICRO 102 > This no longer applies cleanly. [1] http://ffmpeg.org/developer.html#Code-formatting-conventions
On Wed, Sep 4, 2019 at 9:53 AM James Zern <jzern-at-google.com@ffmpeg.org> wrote: > > Hi, > > On Wed, Aug 28, 2019 at 2:26 PM Elliott Karpilovsky > <elliottk-at-google.com@ffmpeg.org> wrote: > > > > Current default is 200kbps, which produces inconsistent > > results (too high for low-res, too low for hi-res). Use > > CRF instead, which will adapt. Affects vp8/vp9. Also > > have VP8 use a default bitrate of 256kbps. > > --- > > libavcodec/libvpxenc.c | 71 +++++++++++++++++++++++++++++++++++------- > > libavcodec/version.h | 2 +- > > 2 files changed, 61 insertions(+), 12 deletions(-) > > > > Some cosmetics, seems to work as expected with tip of tree and v1.4.0. > > > diff --git a/libavcodec/libvpxenc.c b/libavcodec/libvpxenc.c > > index feb52ea0dd..42504ab95e 100644 > > --- a/libavcodec/libvpxenc.c > > +++ b/libavcodec/libvpxenc.c > > @@ -510,6 +510,63 @@ static void set_color_range(AVCodecContext *avctx) > > #endif > > #endif > > > > +/** > > + * Set the target bitrate to VPX library default. Also set CRF to 32 if needed. > > + */ > > +static void set_vp8_defaults(AVCodecContext *avctx, > > + struct vpx_codec_enc_cfg *enccfg) { > > Indent should be 4 and the brace for a function on a new line [1]. > Will send a patch with this fixed. > > [...] > > + > > +/** > > + * Called when the bitrate is not set. It sets appropriate default values for > > + * bit-rate and CRF. > > 'bitrate' is more common in this file. > Ditto. > > [...] > > diff --git a/libavcodec/version.h b/libavcodec/version.h > > index e70ebc0c70..cda6dbae47 100644 > > --- a/libavcodec/version.h > > +++ b/libavcodec/version.h > > @@ -29,7 +29,7 @@ > > > > #define LIBAVCODEC_VERSION_MAJOR 58 > > #define LIBAVCODEC_VERSION_MINOR 55 > > -#define LIBAVCODEC_VERSION_MICRO 101 > > +#define LIBAVCODEC_VERSION_MICRO 102 > > > > This no longer applies cleanly. > Ditto. > [1] http://ffmpeg.org/developer.html#Code-formatting-conventions Thank you for the link. I've updated my editor based on the recommendations there.
diff --git a/libavcodec/libvpxenc.c b/libavcodec/libvpxenc.c index feb52ea0dd..42504ab95e 100644 --- a/libavcodec/libvpxenc.c +++ b/libavcodec/libvpxenc.c @@ -510,6 +510,63 @@ static void set_color_range(AVCodecContext *avctx) #endif #endif +/** + * Set the target bitrate to VPX library default. Also set CRF to 32 if needed. + */ +static void set_vp8_defaults(AVCodecContext *avctx, + struct vpx_codec_enc_cfg *enccfg) { + VPxContext *ctx = avctx->priv_data; + av_assert0(!avctx->bit_rate); + avctx->bit_rate = enccfg->rc_target_bitrate * 1000; + if (enccfg->rc_end_usage == VPX_CQ) { + av_log(avctx, AV_LOG_WARNING, + "Bitrate not specified for constrained quality mode, using default of %dkbit/sec\n", + enccfg->rc_target_bitrate); + } else { + enccfg->rc_end_usage = VPX_CQ; + ctx->crf = 32; + av_log(avctx, AV_LOG_WARNING, + "Neither bitrate nor constrained quality specified, using default CRF of %d and bitrate of %dkbit/sec\n", + ctx->crf, enccfg->rc_target_bitrate); + } +} + + +#if CONFIG_LIBVPX_VP9_ENCODER +/** + * Keep the target bitrate at 0 to engage constant quality mode. If CRF is not + * set, use 32. + */ +static void set_vp9_defaults(AVCodecContext *avctx, + struct vpx_codec_enc_cfg *enccfg) { + VPxContext *ctx = avctx->priv_data; + av_assert0(!avctx->bit_rate); + if (enccfg->rc_end_usage != VPX_Q && ctx->lossless < 0) { + enccfg->rc_end_usage = VPX_Q; + ctx->crf = 32; + av_log(avctx, AV_LOG_WARNING, + "Neither bitrate nor constrained quality specified, using default CRF of %d\n", + ctx->crf); + } +} +#endif + +/** + * Called when the bitrate is not set. It sets appropriate default values for + * bit-rate and CRF. + */ +static void set_vpx_defaults(AVCodecContext *avctx, + struct vpx_codec_enc_cfg *enccfg) { + av_assert0(!avctx->bit_rate); +#if CONFIG_LIBVPX_VP9_ENCODER + if (avctx->codec_id == AV_CODEC_ID_VP9) { + set_vp9_defaults(avctx, enccfg); + return; + } +#endif + set_vp8_defaults(avctx, enccfg); +} + static av_cold int vpx_init(AVCodecContext *avctx, const struct vpx_codec_iface *iface) { @@ -580,18 +637,9 @@ static av_cold int vpx_init(AVCodecContext *avctx, if (avctx->bit_rate) { enccfg.rc_target_bitrate = av_rescale_rnd(avctx->bit_rate, 1, 1000, AV_ROUND_NEAR_INF); -#if CONFIG_LIBVPX_VP9_ENCODER - } else if (enccfg.rc_end_usage == VPX_Q) { -#endif } else { - if (enccfg.rc_end_usage == VPX_CQ) { - enccfg.rc_target_bitrate = 1000000; - } else { - avctx->bit_rate = enccfg.rc_target_bitrate * 1000; - av_log(avctx, AV_LOG_WARNING, - "Neither bitrate nor constrained quality specified, using default bitrate of %dkbit/sec\n", - enccfg.rc_target_bitrate); - } + // Set bit-rate to default value. Also sets CRF to default if needed. + set_vpx_defaults(avctx, &enccfg); } if (avctx->codec_id == AV_CODEC_ID_VP9 && ctx->lossless == 1) { @@ -1266,6 +1314,7 @@ static const AVOption vp9_options[] = { #undef LEGACY_OPTIONS static const AVCodecDefault defaults[] = { + { "b", "0" }, { "qmin", "-1" }, { "qmax", "-1" }, { "g", "-1" }, diff --git a/libavcodec/version.h b/libavcodec/version.h index e70ebc0c70..cda6dbae47 100644 --- a/libavcodec/version.h +++ b/libavcodec/version.h @@ -29,7 +29,7 @@ #define LIBAVCODEC_VERSION_MAJOR 58 #define LIBAVCODEC_VERSION_MINOR 55 -#define LIBAVCODEC_VERSION_MICRO 101 +#define LIBAVCODEC_VERSION_MICRO 102 #define LIBAVCODEC_VERSION_INT AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \ LIBAVCODEC_VERSION_MINOR, \