| Message ID | 6a0e1b12-2267-fd50-db03-d3be3db0a5a8@genshiken.org |
|---|---|
| State | Accepted |
| Headers | show |
On 20 October 2016 at 16:31, Kagami Hiiragi <kagami@genshiken.org> wrote: > vp9_cx_iface actually allows values in range [0..2]. > This fixes ticket #5894. > > Signed-off-by: Kagami Hiiragi <kagami@genshiken.org> > --- > libavcodec/libvpxenc.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/libavcodec/libvpxenc.c b/libavcodec/libvpxenc.c > index 2db87f7..bedaa70 100644 > --- a/libavcodec/libvpxenc.c > +++ b/libavcodec/libvpxenc.c > @@ -615,6 +615,11 @@ FF_ENABLE_DEPRECATION_WARNINGS > } > } > > + if (ctx->auto_alt_ref > 1 && avctx->codec_id == AV_CODEC_ID_VP8) { > + av_log(avctx, AV_LOG_ERROR, "auto_alt_ref > 1 is forbidden for > libvpx-vp8\n"); > + return AVERROR(EINVAL); > + } > + > //codec control failures are currently treated only as warnings > av_log(avctx, AV_LOG_DEBUG, "vpx_codec_control\n"); > codecctl_int(avctx, VP8E_SET_CPUUSED, ctx->cpu_used); > @@ -1025,7 +1030,7 @@ static int vpx_encode(AVCodecContext *avctx, > AVPacket *pkt, > > #define COMMON_OPTIONS \ > { "auto-alt-ref", "Enable use of alternate reference " \ > - "frames (2-pass only)", > OFFSET(auto_alt_ref), AV_OPT_TYPE_BOOL, {.i64 = -1}, -1, 1, > VE}, \ > + "frames (2-pass only)", > OFFSET(auto_alt_ref), AV_OPT_TYPE_INT, {.i64 = -1}, -1, 2, > VE}, \ > { "lag-in-frames", "Number of frames to look ahead for " \ > "alternate reference frame selection", > OFFSET(lag_in_frames), AV_OPT_TYPE_INT, {.i64 = -1}, -1, > INT_MAX, VE}, \ > { "arnr-maxframes", "altref noise reduction max frame count", > OFFSET(arnr_max_frames), AV_OPT_TYPE_INT, {.i64 = -1}, -1, > INT_MAX, VE}, \ > -- > 2.7.3 > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > lgtm You could have moved the check after the tuning ifs before the alpha channel check but it's okay like it is, whoever commits it could do it anyway if they feel like it.
On Thu, Oct 20, 2016 at 8:31 AM, Kagami Hiiragi <kagami@genshiken.org> wrote: > vp9_cx_iface actually allows values in range [0..2]. > This fixes ticket #5894. > > Signed-off-by: Kagami Hiiragi <kagami@genshiken.org> > --- > libavcodec/libvpxenc.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > This is all right in the sense that the library exposes the option. In practice the feature was never fully developed and will cause failures in certain hardware decoders. > diff --git a/libavcodec/libvpxenc.c b/libavcodec/libvpxenc.c > index 2db87f7..bedaa70 100644 > --- a/libavcodec/libvpxenc.c > +++ b/libavcodec/libvpxenc.c > @@ -615,6 +615,11 @@ FF_ENABLE_DEPRECATION_WARNINGS > } > } > > + if (ctx->auto_alt_ref > 1 && avctx->codec_id == AV_CODEC_ID_VP8) { > + av_log(avctx, AV_LOG_ERROR, "auto_alt_ref > 1 is forbidden for libvpx-vp8\n"); > + return AVERROR(EINVAL); > + } > + > //codec control failures are currently treated only as warnings > av_log(avctx, AV_LOG_DEBUG, "vpx_codec_control\n"); > codecctl_int(avctx, VP8E_SET_CPUUSED, ctx->cpu_used); > @@ -1025,7 +1030,7 @@ static int vpx_encode(AVCodecContext *avctx, AVPacket *pkt, > > #define COMMON_OPTIONS \ > { "auto-alt-ref", "Enable use of alternate reference " \ > - "frames (2-pass only)", OFFSET(auto_alt_ref), AV_OPT_TYPE_BOOL, {.i64 = -1}, -1, 1, VE}, \ > + "frames (2-pass only)", OFFSET(auto_alt_ref), AV_OPT_TYPE_INT, {.i64 = -1}, -1, 2, VE}, \ > { "lag-in-frames", "Number of frames to look ahead for " \ > "alternate reference frame selection", OFFSET(lag_in_frames), AV_OPT_TYPE_INT, {.i64 = -1}, -1, INT_MAX, VE}, \ > { "arnr-maxframes", "altref noise reduction max frame count", OFFSET(arnr_max_frames), AV_OPT_TYPE_INT, {.i64 = -1}, -1, INT_MAX, VE}, \ > -- > 2.7.3 > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On Thu, Oct 20, 2016 at 11:51 AM, Rostislav Pehlivanov <atomnuker@gmail.com> wrote: > On 20 October 2016 at 16:31, Kagami Hiiragi <kagami@genshiken.org> wrote: > >> vp9_cx_iface actually allows values in range [0..2]. >> This fixes ticket #5894. >> >> Signed-off-by: Kagami Hiiragi <kagami@genshiken.org> >> --- >> libavcodec/libvpxenc.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/libavcodec/libvpxenc.c b/libavcodec/libvpxenc.c >> index 2db87f7..bedaa70 100644 >> --- a/libavcodec/libvpxenc.c >> +++ b/libavcodec/libvpxenc.c >> @@ -615,6 +615,11 @@ FF_ENABLE_DEPRECATION_WARNINGS >> } >> } >> >> + if (ctx->auto_alt_ref > 1 && avctx->codec_id == AV_CODEC_ID_VP8) { >> + av_log(avctx, AV_LOG_ERROR, "auto_alt_ref > 1 is forbidden for >> libvpx-vp8\n"); >> + return AVERROR(EINVAL); >> + } >> + >> //codec control failures are currently treated only as warnings >> av_log(avctx, AV_LOG_DEBUG, "vpx_codec_control\n"); >> codecctl_int(avctx, VP8E_SET_CPUUSED, ctx->cpu_used); >> @@ -1025,7 +1030,7 @@ static int vpx_encode(AVCodecContext *avctx, >> AVPacket *pkt, >> >> #define COMMON_OPTIONS \ >> { "auto-alt-ref", "Enable use of alternate reference " \ >> - "frames (2-pass only)", >> OFFSET(auto_alt_ref), AV_OPT_TYPE_BOOL, {.i64 = -1}, -1, 1, >> VE}, \ >> + "frames (2-pass only)", >> OFFSET(auto_alt_ref), AV_OPT_TYPE_INT, {.i64 = -1}, -1, 2, >> VE}, \ >> { "lag-in-frames", "Number of frames to look ahead for " \ >> "alternate reference frame selection", >> OFFSET(lag_in_frames), AV_OPT_TYPE_INT, {.i64 = -1}, -1, >> INT_MAX, VE}, \ >> { "arnr-maxframes", "altref noise reduction max frame count", >> OFFSET(arnr_max_frames), AV_OPT_TYPE_INT, {.i64 = -1}, -1, >> INT_MAX, VE}, \ >> -- >> 2.7.3 >> >> _______________________________________________ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> > > lgtm > You could have moved the check after the tuning ifs before the alpha > channel check but it's okay like it is, whoever commits it could do it > anyway if they feel like it. > The library will report an error (treated as a warning here) which is probably enough.
On 21/10/16 06:14, James Zern wrote: > On Thu, Oct 20, 2016 at 8:31 AM, Kagami Hiiragi <kagami@genshiken.org> wrote: >> vp9_cx_iface actually allows values in range [0..2]. >> This fixes ticket #5894. >> >> Signed-off-by: Kagami Hiiragi <kagami@genshiken.org> >> --- >> libavcodec/libvpxenc.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> > > This is all right in the sense that the library exposes the option. In > practice the feature was never fully developed and will cause failures > in certain hardware decoders. vpxenc exposes it, good enough reason to allow that in ffmpeg too. > The library will report an error (treated as a warning here) which is > probably enough. Passing wrong option to encoder should be treated as error in my opinion. Without that check library will just report warning and encoding process won't be halted.
On Fri, Oct 21, 2016 at 1:14 AM, Kagami Hiiragi <kagami@genshiken.org> wrote: > On 21/10/16 06:14, James Zern wrote: >> On Thu, Oct 20, 2016 at 8:31 AM, Kagami Hiiragi <kagami@genshiken.org> wrote: >>> vp9_cx_iface actually allows values in range [0..2]. >>> This fixes ticket #5894. >>> >>> Signed-off-by: Kagami Hiiragi <kagami@genshiken.org> >>> --- >>> libavcodec/libvpxenc.c | 7 ++++++- >>> 1 file changed, 6 insertions(+), 1 deletion(-) >>> >> >> This is all right in the sense that the library exposes the option. In >> practice the feature was never fully developed and will cause failures >> in certain hardware decoders. > > vpxenc exposes it, good enough reason to allow that in ffmpeg too. > > >> The library will report an error (treated as a warning here) which is >> probably enough. > > Passing wrong option to encoder should be treated as error in my > opinion. Without that check library will just report warning and > encoding process won't be halted. > That's the result for every other control we don't constrain. For now the local check is fine, but moving forward we should probably fail the control now that we don't try to set them without the user explicitly setting an option.
Hi, On Thu, Oct 20, 2016 at 11:14 PM, James Zern <jzern-at-google.com@ffmpeg.org > wrote: > On Thu, Oct 20, 2016 at 8:31 AM, Kagami Hiiragi <kagami@genshiken.org> > wrote: > > vp9_cx_iface actually allows values in range [0..2]. > > This fixes ticket #5894. > > > > Signed-off-by: Kagami Hiiragi <kagami@genshiken.org> > > --- > > libavcodec/libvpxenc.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > This is all right in the sense that the library exposes the option. In > practice the feature was never fully developed and will cause failures > in certain hardware decoders. Maybe these hardware should be blacklisted. Ronald
On Fri, Oct 21, 2016 at 1:22 AM, James Zern <jzern@google.com> wrote: > On Fri, Oct 21, 2016 at 1:14 AM, Kagami Hiiragi <kagami@genshiken.org> wrote: >> On 21/10/16 06:14, James Zern wrote: >>> On Thu, Oct 20, 2016 at 8:31 AM, Kagami Hiiragi <kagami@genshiken.org> wrote: >>>> vp9_cx_iface actually allows values in range [0..2]. >>>> This fixes ticket #5894. >>>> >>>> Signed-off-by: Kagami Hiiragi <kagami@genshiken.org> >>>> --- >>>> libavcodec/libvpxenc.c | 7 ++++++- >>>> 1 file changed, 6 insertions(+), 1 deletion(-) >>>> >>> >>> This is all right in the sense that the library exposes the option. In >>> practice the feature was never fully developed and will cause failures >>> in certain hardware decoders. >> >> vpxenc exposes it, good enough reason to allow that in ffmpeg too. >> >> >>> The library will report an error (treated as a warning here) which is >>> probably enough. >> >> Passing wrong option to encoder should be treated as error in my >> opinion. Without that check library will just report warning and >> encoding process won't be halted. >> > > That's the result for every other control we don't constrain. For now > the local check is fine, but moving forward we should probably fail > the control now that we don't try to set them without the user > explicitly setting an option. > I think it may just be easier to adapt this one rather than throw an error.
On Fri, Oct 21, 2016 at 4:55 AM, Ronald S. Bultje <rsbultje@gmail.com> wrote: > Hi, > > On Thu, Oct 20, 2016 at 11:14 PM, James Zern <jzern-at-google.com@ffmpeg.org >> wrote: > >> On Thu, Oct 20, 2016 at 8:31 AM, Kagami Hiiragi <kagami@genshiken.org> >> wrote: >> > vp9_cx_iface actually allows values in range [0..2]. >> > This fixes ticket #5894. >> > >> > Signed-off-by: Kagami Hiiragi <kagami@genshiken.org> >> > --- >> > libavcodec/libvpxenc.c | 7 ++++++- >> > 1 file changed, 6 insertions(+), 1 deletion(-) >> > >> >> This is all right in the sense that the library exposes the option. In >> practice the feature was never fully developed and will cause failures >> in certain hardware decoders. > > > Maybe these hardware should be blacklisted. > I don't necessarily disagree, but I think issues with multiple altrefs and/or the frequency of individual ones is somewhat pervasive. So blacklisting might mean vp9 wasn't an option on some platforms. This is part of the reason behind all this [1]. [1] http://www.webmproject.org/vp9/levels/
diff --git a/libavcodec/libvpxenc.c b/libavcodec/libvpxenc.c index 2db87f7..bedaa70 100644 --- a/libavcodec/libvpxenc.c +++ b/libavcodec/libvpxenc.c @@ -615,6 +615,11 @@ FF_ENABLE_DEPRECATION_WARNINGS } } + if (ctx->auto_alt_ref > 1 && avctx->codec_id == AV_CODEC_ID_VP8) { + av_log(avctx, AV_LOG_ERROR, "auto_alt_ref > 1 is forbidden for libvpx-vp8\n"); + return AVERROR(EINVAL); + } + //codec control failures are currently treated only as warnings av_log(avctx, AV_LOG_DEBUG, "vpx_codec_control\n"); codecctl_int(avctx, VP8E_SET_CPUUSED, ctx->cpu_used); @@ -1025,7 +1030,7 @@ static int vpx_encode(AVCodecContext *avctx, AVPacket *pkt, #define COMMON_OPTIONS \ { "auto-alt-ref", "Enable use of alternate reference " \ - "frames (2-pass only)", OFFSET(auto_alt_ref), AV_OPT_TYPE_BOOL, {.i64 = -1}, -1, 1, VE}, \ + "frames (2-pass only)", OFFSET(auto_alt_ref), AV_OPT_TYPE_INT, {.i64 = -1}, -1, 2, VE}, \ { "lag-in-frames", "Number of frames to look ahead for " \ "alternate reference frame selection", OFFSET(lag_in_frames), AV_OPT_TYPE_INT, {.i64 = -1}, -1, INT_MAX, VE}, \ { "arnr-maxframes", "altref noise reduction max frame count", OFFSET(arnr_max_frames), AV_OPT_TYPE_INT, {.i64 = -1}, -1, INT_MAX, VE}, \
vp9_cx_iface actually allows values in range [0..2]. This fixes ticket #5894. Signed-off-by: Kagami Hiiragi <kagami@genshiken.org> --- libavcodec/libvpxenc.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)