| Message ID | 0e2faff5-2182-6d83-bae1-8f5195ec0792@mediaarea.net |
|---|---|
| State | New |
| Headers | show |
On Wed, Mar 07, 2018 at 04:49:24PM +0100, Jerome Martinez wrote: > There is a message when coder type is forced to a value not chosen by user, > but no message when version is forced to a value not chosen by user. > This patch adds such message for more coherency in the messages, and the > user is informed that the command is not fully respected. > > ffmpeg f lavfi -i mandelbrot=s=1920x1080 -vf format=gbrp9 -vframes 1 -c ffv1 > -level 0 -coder 0 a.mkv > > Before: > [ffv1 @ 000002492CD69B40] bits_per_raw_sample > 8, forcing range coder > > After: > [ffv1 @ 000001A6E404A780] bits_per_raw_sample > 8, forcing version 1 > [ffv1 @ 000001A6E404A780] bits_per_raw_sample > 8, forcing range coder > > > ffv1enc.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > cb1df919e21fe4d388df7de9349c5c2c46777862 0002-avcodec-ffv1enc-add-information-message-when-version.patch > From 49db6049fa50976683fc651cf180ab8c7428225e Mon Sep 17 00:00:00 2001 > From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?= <jerome@mediaarea.net> > Date: Wed, 7 Mar 2018 10:37:46 +0100 > Subject: [PATCH 2/7] avcodec/ffv1enc: add information message when version is > changed by the encoder > > --- > libavcodec/ffv1enc.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/libavcodec/ffv1enc.c b/libavcodec/ffv1enc.c > index d71d952c6d..ac8b715b74 100644 > --- a/libavcodec/ffv1enc.c > +++ b/libavcodec/ffv1enc.c > @@ -509,7 +509,7 @@ static av_cold int encode_init(AVCodecContext *avctx) > { > FFV1Context *s = avctx->priv_data; > const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(avctx->pix_fmt); > - int i, j, k, m, ret; > + int i, j, k, m, ret, oldversion; > > if ((ret = ff_ffv1_common_init(avctx)) < 0) > return ret; > @@ -534,6 +534,7 @@ static av_cold int encode_init(AVCodecContext *avctx) > } > s->version = avctx->level; > } > + oldversion = s->version; > > if (s->ec < 0) { > s->ec = (s->version >= 3); > @@ -679,6 +680,11 @@ FF_ENABLE_DEPRECATION_WARNINGS > av_assert0(s->bits_per_raw_sample >= 8); > > if (s->bits_per_raw_sample > 8) { > + if (oldversion >= 0 && oldversion != s->version) { > + av_log(avctx, AV_LOG_INFO, > + "bits_per_raw_sample > 8, forcing version 1\n"); > + oldversion = s->version; > + } I dont think this works consistently The code does not seem to be limited to the case where the user has specifified a version for example unless i miss something [...]
On 08/03/2018 01:33, Michael Niedermayer wrote: > On Wed, Mar 07, 2018 at 04:49:24PM +0100, Jerome Martinez wrote: >> There is a message when coder type is forced to a value not chosen by user, >> but no message when version is forced to a value not chosen by user. >> This patch adds such message for more coherency in the messages, and the >> user is informed that the command is not fully respected. >> >> ffmpeg f lavfi -i mandelbrot=s=1920x1080 -vf format=gbrp9 -vframes 1 -c ffv1 >> -level 0 -coder 0 a.mkv >> >> Before: >> [ffv1 @ 000002492CD69B40] bits_per_raw_sample > 8, forcing range coder >> >> After: >> [ffv1 @ 000001A6E404A780] bits_per_raw_sample > 8, forcing version 1 >> [ffv1 @ 000001A6E404A780] bits_per_raw_sample > 8, forcing range coder >> >> >> ffv1enc.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> cb1df919e21fe4d388df7de9349c5c2c46777862 0002-avcodec-ffv1enc-add-information-message-when-version.patch >> From 49db6049fa50976683fc651cf180ab8c7428225e Mon Sep 17 00:00:00 2001 >> From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?= <jerome@mediaarea.net> >> Date: Wed, 7 Mar 2018 10:37:46 +0100 >> Subject: [PATCH 2/7] avcodec/ffv1enc: add information message when version is >> changed by the encoder >> >> --- >> libavcodec/ffv1enc.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/libavcodec/ffv1enc.c b/libavcodec/ffv1enc.c >> index d71d952c6d..ac8b715b74 100644 >> --- a/libavcodec/ffv1enc.c >> +++ b/libavcodec/ffv1enc.c >> @@ -509,7 +509,7 @@ static av_cold int encode_init(AVCodecContext *avctx) >> { >> FFV1Context *s = avctx->priv_data; >> const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(avctx->pix_fmt); >> - int i, j, k, m, ret; >> + int i, j, k, m, ret, oldversion; >> >> if ((ret = ff_ffv1_common_init(avctx)) < 0) >> return ret; >> @@ -534,6 +534,7 @@ static av_cold int encode_init(AVCodecContext *avctx) >> } >> s->version = avctx->level; >> } >> + oldversion = s->version; >> >> if (s->ec < 0) { >> s->ec = (s->version >= 3); >> @@ -679,6 +680,11 @@ FF_ENABLE_DEPRECATION_WARNINGS >> av_assert0(s->bits_per_raw_sample >= 8); >> >> if (s->bits_per_raw_sample > 8) { >> + if (oldversion >= 0 && oldversion != s->version) { >> + av_log(avctx, AV_LOG_INFO, >> + "bits_per_raw_sample > 8, forcing version 1\n"); >> + oldversion = s->version; >> + } > I dont think this works consistently > > The code does not seem to be limited to the case where the user has > specifified a version for example unless i miss something checking range coder part, I see that currently there is actually a slight difference with the other AV_LOG_INFO, I don't indicate the message when level is not indicated, as I didn't see the value added of such information when level is not indicated on the command line when I implemented the test. With patch proposal: ffmpeg f lavfi -i mandelbrot=s=1920x1080 -vf format=gbrp9 -vframes 1 -c ffv1 a.mkv [ffv1 @ 00000232D35B9240] bits_per_raw_sample > 8, forcing range coder (no change) ffmpeg f lavfi -i mandelbrot=s=1920x1080 -vf format=gbrp9 -vframes 1 -c ffv1 -coder 0 a.mkv [ffv1 @ 0000014B9B1771C0] bits_per_raw_sample > 8, forcing range coder (no change) ffmpeg f lavfi -i mandelbrot=s=1920x1080 -vf format=gbrp9 -vframes 1 -c ffv1 -level 0 a.mkv [ffv1 @ 000001BC2B881500] bits_per_raw_sample > 8, forcing version 1 [ffv1 @ 000001BC2B881500] bits_per_raw_sample > 8, forcing range coder ("version 1" line added because user specified version 0, IMO encoder should inform user that this is not respected) ffmpeg f lavfi -i mandelbrot=s=1920x1080 -vf format=gbrp9 -vframes 1 -c ffv1 -level 0 -coder 0 a.mkv [ffv1 @ 000001EAC7685180] bits_per_raw_sample > 8, forcing version 1 [ffv1 @ 000001EAC7685180] bits_per_raw_sample > 8, forcing range coder ("version 1" line added because user specified version 0, IMO encoder should inform user that this is not respected) Just for being clear: do you want info message also when level/version is *not* indicated on the command line?
On Thu, Mar 08, 2018 at 03:12:26AM +0100, Jerome Martinez wrote: > On 08/03/2018 01:33, Michael Niedermayer wrote: > >On Wed, Mar 07, 2018 at 04:49:24PM +0100, Jerome Martinez wrote: > >>There is a message when coder type is forced to a value not chosen by user, > >>but no message when version is forced to a value not chosen by user. > >>This patch adds such message for more coherency in the messages, and the > >>user is informed that the command is not fully respected. > >> > >>ffmpeg f lavfi -i mandelbrot=s=1920x1080 -vf format=gbrp9 -vframes 1 -c ffv1 > >>-level 0 -coder 0 a.mkv > >> > >>Before: > >>[ffv1 @ 000002492CD69B40] bits_per_raw_sample > 8, forcing range coder > >> > >>After: > >>[ffv1 @ 000001A6E404A780] bits_per_raw_sample > 8, forcing version 1 > >>[ffv1 @ 000001A6E404A780] bits_per_raw_sample > 8, forcing range coder > >> > >> > >> ffv1enc.c | 8 +++++++- > >> 1 file changed, 7 insertions(+), 1 deletion(-) > >>cb1df919e21fe4d388df7de9349c5c2c46777862 0002-avcodec-ffv1enc-add-information-message-when-version.patch > >> From 49db6049fa50976683fc651cf180ab8c7428225e Mon Sep 17 00:00:00 2001 > >>From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?= <jerome@mediaarea.net> > >>Date: Wed, 7 Mar 2018 10:37:46 +0100 > >>Subject: [PATCH 2/7] avcodec/ffv1enc: add information message when version is > >> changed by the encoder > >> > >>--- > >> libavcodec/ffv1enc.c | 8 +++++++- > >> 1 file changed, 7 insertions(+), 1 deletion(-) > >> > >>diff --git a/libavcodec/ffv1enc.c b/libavcodec/ffv1enc.c > >>index d71d952c6d..ac8b715b74 100644 > >>--- a/libavcodec/ffv1enc.c > >>+++ b/libavcodec/ffv1enc.c > >>@@ -509,7 +509,7 @@ static av_cold int encode_init(AVCodecContext *avctx) > >> { > >> FFV1Context *s = avctx->priv_data; > >> const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(avctx->pix_fmt); > >>- int i, j, k, m, ret; > >>+ int i, j, k, m, ret, oldversion; > >> if ((ret = ff_ffv1_common_init(avctx)) < 0) > >> return ret; > >>@@ -534,6 +534,7 @@ static av_cold int encode_init(AVCodecContext *avctx) > >> } > >> s->version = avctx->level; > >> } > >>+ oldversion = s->version; > >> if (s->ec < 0) { > >> s->ec = (s->version >= 3); > >>@@ -679,6 +680,11 @@ FF_ENABLE_DEPRECATION_WARNINGS > >> av_assert0(s->bits_per_raw_sample >= 8); > >> if (s->bits_per_raw_sample > 8) { > >>+ if (oldversion >= 0 && oldversion != s->version) { > >>+ av_log(avctx, AV_LOG_INFO, > >>+ "bits_per_raw_sample > 8, forcing version 1\n"); > >>+ oldversion = s->version; > >>+ } > >I dont think this works consistently > > > >The code does not seem to be limited to the case where the user has > >specifified a version for example unless i miss something > > checking range coder part, I see that currently there is actually a slight > difference with the other AV_LOG_INFO, I don't indicate the message when > level is not indicated, as I didn't see the value added of such information > when level is not indicated on the command line when I implemented the test. > > With patch proposal: > > ffmpeg f lavfi -i mandelbrot=s=1920x1080 -vf format=gbrp9 -vframes 1 -c ffv1 > a.mkv > [ffv1 @ 00000232D35B9240] bits_per_raw_sample > 8, forcing range coder > (no change) > > ffmpeg f lavfi -i mandelbrot=s=1920x1080 -vf format=gbrp9 -vframes 1 -c ffv1 > -coder 0 a.mkv > [ffv1 @ 0000014B9B1771C0] bits_per_raw_sample > 8, forcing range coder > (no change) > > ffmpeg f lavfi -i mandelbrot=s=1920x1080 -vf format=gbrp9 -vframes 1 -c ffv1 > -level 0 a.mkv > [ffv1 @ 000001BC2B881500] bits_per_raw_sample > 8, forcing version 1 > [ffv1 @ 000001BC2B881500] bits_per_raw_sample > 8, forcing range coder > ("version 1" line added because user specified version 0, IMO encoder should > inform user that this is not respected) > > ffmpeg f lavfi -i mandelbrot=s=1920x1080 -vf format=gbrp9 -vframes 1 -c ffv1 > -level 0 -coder 0 a.mkv > [ffv1 @ 000001EAC7685180] bits_per_raw_sample > 8, forcing version 1 > [ffv1 @ 000001EAC7685180] bits_per_raw_sample > 8, forcing range coder > ("version 1" line added because user specified version 0, IMO encoder should > inform user that this is not respected) I have the suspicion there are more corner cases that behave odd, but ive no time ATM to test, ill do that tomorrow unless i forget > > Just for being clear: do you want info message also when level/version is > *not* indicated on the command line? some sort of debug level message indicating the version may be usefull if theres nothing showing it currently at higher level i think it should only be shown if its worth it to the user like for example if it overrides what was speified or somehow has compatibility issues [...]
On 09/03/2018 03:15, Michael Niedermayer wrote: > On Thu, Mar 08, 2018 at 03:12:26AM +0100, Jerome Martinez wrote: >> >> checking range coder part, I see that currently there is actually a slight >> difference with the other AV_LOG_INFO, I don't indicate the message when >> level is not indicated, as I didn't see the value added of such information >> when level is not indicated on the command line when I implemented the test. >> >> With patch proposal: >> >> ffmpeg f lavfi -i mandelbrot=s=1920x1080 -vf format=gbrp9 -vframes 1 -c ffv1 >> a.mkv >> [ffv1 @ 00000232D35B9240] bits_per_raw_sample > 8, forcing range coder >> (no change) >> >> ffmpeg f lavfi -i mandelbrot=s=1920x1080 -vf format=gbrp9 -vframes 1 -c ffv1 >> -coder 0 a.mkv >> [ffv1 @ 0000014B9B1771C0] bits_per_raw_sample > 8, forcing range coder >> (no change) >> >> ffmpeg f lavfi -i mandelbrot=s=1920x1080 -vf format=gbrp9 -vframes 1 -c ffv1 >> -level 0 a.mkv >> [ffv1 @ 000001BC2B881500] bits_per_raw_sample > 8, forcing version 1 >> [ffv1 @ 000001BC2B881500] bits_per_raw_sample > 8, forcing range coder >> ("version 1" line added because user specified version 0, IMO encoder should >> inform user that this is not respected) >> >> ffmpeg f lavfi -i mandelbrot=s=1920x1080 -vf format=gbrp9 -vframes 1 -c ffv1 >> -level 0 -coder 0 a.mkv >> [ffv1 @ 000001EAC7685180] bits_per_raw_sample > 8, forcing version 1 >> [ffv1 @ 000001EAC7685180] bits_per_raw_sample > 8, forcing range coder >> ("version 1" line added because user specified version 0, IMO encoder should >> inform user that this is not respected) > I have the suspicion there are more corner cases that behave odd, but ive > no time ATM to test, ill do that tomorrow unless i forget I'll add some tests about respecting or displaying the change compared to the command line in FATE. > > >> Just for being clear: do you want info message also when level/version is >> *not* indicated on the command line? > some sort of debug level message indicating the version may be usefull > if theres nothing showing it currently For debug log, I suggest to provide all impacting config items, i.e. version, slice count, coder type... > > at higher level i think it should only be shown if its worth it to the > user like for example if it overrides what was speified or somehow has > compatibility issues So my patch is fine, and there is the need to change the current behavior for the other log line, I'll do another patch. But for the moment I prefer to have the already FFV1 related pending items (7 FFmpeg patches and 5 FFV1 spec patches) handled for better seeing the direction to go before adding new items to the list.
diff --git a/libavcodec/ffv1enc.c b/libavcodec/ffv1enc.c index d71d952c6d..ac8b715b74 100644 --- a/libavcodec/ffv1enc.c +++ b/libavcodec/ffv1enc.c @@ -509,7 +509,7 @@ static av_cold int encode_init(AVCodecContext *avctx) { FFV1Context *s = avctx->priv_data; const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(avctx->pix_fmt); - int i, j, k, m, ret; + int i, j, k, m, ret, oldversion; if ((ret = ff_ffv1_common_init(avctx)) < 0) return ret; @@ -534,6 +534,7 @@ static av_cold int encode_init(AVCodecContext *avctx) } s->version = avctx->level; } + oldversion = s->version; if (s->ec < 0) { s->ec = (s->version >= 3); @@ -679,6 +680,11 @@ FF_ENABLE_DEPRECATION_WARNINGS av_assert0(s->bits_per_raw_sample >= 8); if (s->bits_per_raw_sample > 8) { + if (oldversion >= 0 && oldversion != s->version) { + av_log(avctx, AV_LOG_INFO, + "bits_per_raw_sample > 8, forcing version 1\n"); + oldversion = s->version; + } if (s->ac == AC_GOLOMB_RICE) { av_log(avctx, AV_LOG_INFO, "bits_per_raw_sample > 8, forcing range coder\n");
There is a message when coder type is forced to a value not chosen by user, but no message when version is forced to a value not chosen by user. This patch adds such message for more coherency in the messages, and the user is informed that the command is not fully respected. ffmpeg f lavfi -i mandelbrot=s=1920x1080 -vf format=gbrp9 -vframes 1 -c ffv1 -level 0 -coder 0 a.mkv Before: [ffv1 @ 000002492CD69B40] bits_per_raw_sample > 8, forcing range coder After: [ffv1 @ 000001A6E404A780] bits_per_raw_sample > 8, forcing version 1 [ffv1 @ 000001A6E404A780] bits_per_raw_sample > 8, forcing range coder From 49db6049fa50976683fc651cf180ab8c7428225e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Martinez?= <jerome@mediaarea.net> Date: Wed, 7 Mar 2018 10:37:46 +0100 Subject: [PATCH 2/7] avcodec/ffv1enc: add information message when version is changed by the encoder --- libavcodec/ffv1enc.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)