[FFmpeg-devel,2/7] avcodec/ffv1enc: add information message when version is changed by the encoder

Submitted by Jerome Martinez on March 7, 2018, 3:49 p.m.

Details

Message ID 0e2faff5-2182-6d83-bae1-8f5195ec0792@mediaarea.net
State New
Headers show

Commit Message

Jerome Martinez March 7, 2018, 3:49 p.m.
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(-)

Comments

Michael Niedermayer March 8, 2018, 12:33 a.m.
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


[...]
Jerome Martinez March 8, 2018, 2:12 a.m.
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?
Michael Niedermayer March 9, 2018, 2:15 a.m.
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

[...]
Jerome Martinez March 9, 2018, 2:18 p.m.
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.

Patch hide | download patch | download mbox

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");