[FFmpeg-devel] [RFC/PATCH] fftools/ffmpeg: stop printing PSNR information in status report

[FFmpeg-devel] [RFC/PATCH] fftools/ffmpeg: stop printing PSNR information in status report

[Anton Khirnov]

When an encoder exports sum-of-squared-differences information in
encoded packets, print_report() will print PSNR information in the
status line. However,
* the code computing PSNR assumes 8bit 420 video and prints incorrect
  values otherwise; there are no issues on trac about this
* only a few encoders (namely aom, vpx, mpegvideo, snow) export this
  information; other often-used encoders such as libx26[45] do not
  export this, even though they could

This suggests that this feature is not useful and it is better to remove
it rather than spend effort on fixing it.
---
I needed to resolve this code's interaction with encoders as a part of
my multithreading work and spent a few hours on it. Making it work
correctly in all cases seems nontrivial and duplicates a lot of the
logic from vf_psnr.

Given that nobody ever noticed that it's broken for everything other
than YUV420P, or bothered adding support in libx264 strongly suggests to
me that nobody cares about this and it can be safely dropped.

Anyone against?
---
 fftools/ffmpeg.c | 30 ------------------------------
 1 file changed, 30 deletions(-)

diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
index a7e856e9e2..c73064af9a 100644
--- a/fftools/ffmpeg.c
+++ b/fftools/ffmpeg.c
@@ -767,36 +767,6 @@ static void print_report(int is_last_report, int64_t timer_start, int64_t cur_ti
             if (is_last_report)
                 av_bprintf(&buf, "L");
 
-            if (enc && (enc->flags & AV_CODEC_FLAG_PSNR) &&
-                (ost->pict_type != AV_PICTURE_TYPE_NONE || is_last_report)) {
-                int j;
-                double error, error_sum = 0;
-                double scale, scale_sum = 0;
-                double p;
-                char type[3] = { 'Y','U','V' };
-                av_bprintf(&buf, "PSNR=");
-                for (j = 0; j < 3; j++) {
-                    if (is_last_report) {
-                        error = enc->error[j];
-                        scale = enc->width * enc->height * 255.0 * 255.0 * frame_number;
-                    } else {
-                        error = ost->error[j];
-                        scale = enc->width * enc->height * 255.0 * 255.0;
-                    }
-                    if (j)
-                        scale /= 4;
-                    error_sum += error;
-                    scale_sum += scale;
-                    p = psnr(error / scale);
-                    av_bprintf(&buf, "%c:%2.2f ", type[j], p);
-                    av_bprintf(&buf_script, "stream_%d_%d_psnr_%c=%2.2f\n",
-                               ost->file_index, ost->index, type[j] | 32, p);
-                }
-                p = psnr(error_sum / scale_sum);
-                av_bprintf(&buf, "*:%2.2f ", psnr(error_sum / scale_sum));
-                av_bprintf(&buf_script, "stream_%d_%d_psnr_all=%2.2f\n",
-                           ost->file_index, ost->index, p);
-            }
             vid = 1;
         }
         /* compute min output value */
-- 
2.39.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [RFC/PATCH] fftools/ffmpeg: stop printing PSNR information in status report

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1559 bytes --]

On Wed, Apr 19, 2023 at 09:48:32PM +0200, Anton Khirnov wrote:
> When an encoder exports sum-of-squared-differences information in
> encoded packets, print_report() will print PSNR information in the
> status line. However,

> * the code computing PSNR assumes 8bit 420 video and prints incorrect
>   values otherwise; there are no issues on trac about this

Are the values in the "otherwise" case maybe good enough so they
worked for people with noone noticing ?


> * only a few encoders (namely aom, vpx, mpegvideo, snow) export this
>   information; other often-used encoders such as libx26[45] do not
>   export this, even though they could
> 
> This suggests that this feature is not useful and it is better to remove
> it rather than spend effort on fixing it.
> ---
> I needed to resolve this code's interaction with encoders as a part of
> my multithreading work and spent a few hours on it. Making it work
> correctly in all cases seems nontrivial and duplicates a lot of the
> logic from vf_psnr.

Can anything missing in vf_psnr be added into it and then vf_psnr used ?
I agree that duplicating PSNR code and logic is bad


> 
> Given that nobody ever noticed that it's broken for everything other
> than YUV420P, or bothered adding support in libx264 strongly suggests to
> me that nobody cares about this and it can be safely dropped.
> 
> Anyone against?

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No snowflake in an avalanche ever feels responsible. -- Voltaire

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [RFC/PATCH] fftools/ffmpeg: stop printing PSNR information in status report

[Anton Khirnov]

Quoting Michael Niedermayer (2023-04-19 22:53:02)
> On Wed, Apr 19, 2023 at 09:48:32PM +0200, Anton Khirnov wrote:
> > When an encoder exports sum-of-squared-differences information in
> > encoded packets, print_report() will print PSNR information in the
> > status line. However,
> 
> > * the code computing PSNR assumes 8bit 420 video and prints incorrect
> >   values otherwise; there are no issues on trac about this
> 
> Are the values in the "otherwise" case maybe good enough so they
> worked for people with noone noticing ?

While working on this with a 10bit sample I was suprised this code and
vf_psnr showed significantly different values (IIRC not even the first
digit was accurate) and it took me a while to realize the scaling made
the assumptions it did.

> > * only a few encoders (namely aom, vpx, mpegvideo, snow) export this
> >   information; other often-used encoders such as libx26[45] do not
> >   export this, even though they could
> > 
> > This suggests that this feature is not useful and it is better to remove
> > it rather than spend effort on fixing it.
> > ---
> > I needed to resolve this code's interaction with encoders as a part of
> > my multithreading work and spent a few hours on it. Making it work
> > correctly in all cases seems nontrivial and duplicates a lot of the
> > logic from vf_psnr.
> 
> Can anything missing in vf_psnr be added into it and then vf_psnr used ?
> I agree that duplicating PSNR code and logic is bad

Nothing is missing in vf_psnr AFAIK, the difference is that these values
are produced directly by the encoder, so you don't need a
decoding+filtering pass to obtain the numbers.

The question more broadly is - what is this supposed to be useful for?
PSNR is a highly flawed metric and AFAIU state of the art moved several
generations away from it. And for a quick-and-dirty quality estimate,
the bitrate and QP might be more informative for most users, and are
supported by more encoders. So what is the point of printing this
information?

-- 
Anton Khirnov
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [RFC/PATCH] fftools/ffmpeg: stop printing PSNR information in status report

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 2993 bytes --]

On Wed, Apr 19, 2023 at 11:06:09PM +0200, Anton Khirnov wrote:
> Quoting Michael Niedermayer (2023-04-19 22:53:02)
> > On Wed, Apr 19, 2023 at 09:48:32PM +0200, Anton Khirnov wrote:
> > > When an encoder exports sum-of-squared-differences information in
> > > encoded packets, print_report() will print PSNR information in the
> > > status line. However,
> > 
> > > * the code computing PSNR assumes 8bit 420 video and prints incorrect
> > >   values otherwise; there are no issues on trac about this
> > 
> > Are the values in the "otherwise" case maybe good enough so they
> > worked for people with noone noticing ?
> 
> While working on this with a 10bit sample I was suprised this code and
> vf_psnr showed significantly different values (IIRC not even the first
> digit was accurate) and it took me a while to realize the scaling made
> the assumptions it did.
> 
> > > * only a few encoders (namely aom, vpx, mpegvideo, snow) export this
> > >   information; other often-used encoders such as libx26[45] do not
> > >   export this, even though they could
> > > 
> > > This suggests that this feature is not useful and it is better to remove
> > > it rather than spend effort on fixing it.
> > > ---
> > > I needed to resolve this code's interaction with encoders as a part of
> > > my multithreading work and spent a few hours on it. Making it work
> > > correctly in all cases seems nontrivial and duplicates a lot of the
> > > logic from vf_psnr.
> > 
> > Can anything missing in vf_psnr be added into it and then vf_psnr used ?
> > I agree that duplicating PSNR code and logic is bad
> 
> Nothing is missing in vf_psnr AFAIK, the difference is that these values
> are produced directly by the encoder, so you don't need a
> decoding+filtering pass to obtain the numbers.

That sounds like a missing feature.
vf_psnr cannot use the encoder values, it always needs to recompute them.
The encoder could export these values in metadata and vf_psnr could
then check if the frame pairs already have their psnr computed and use
that.


> 
> The question more broadly is - what is this supposed to be useful for?
> PSNR is a highly flawed metric and AFAIU state of the art moved several
> generations away from it. And for a quick-and-dirty quality estimate,
> the bitrate and QP might be more informative for most users, and are
> supported by more encoders. So what is the point of printing this
> information?

The "problem" isnt psnr specific
having a filter which could provide another metric and be able to use
encoder supplied data when available or compute them from encoder input +
decoded images in a way that is automatic would be useful
It would mean a single syntax a user could use to get the metric he wants
while using the optimal implementation

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Those who are best at talking, realize last or never when they are wrong.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [RFC/PATCH] fftools/ffmpeg: stop printing PSNR information in status report

[Anton Khirnov]

Quoting Michael Niedermayer (2023-04-20 00:12:48)
> On Wed, Apr 19, 2023 at 11:06:09PM +0200, Anton Khirnov wrote:
> > Quoting Michael Niedermayer (2023-04-19 22:53:02)
> > > On Wed, Apr 19, 2023 at 09:48:32PM +0200, Anton Khirnov wrote:
> > > > When an encoder exports sum-of-squared-differences information in
> > > > encoded packets, print_report() will print PSNR information in the
> > > > status line. However,
> > > 
> > > > * the code computing PSNR assumes 8bit 420 video and prints incorrect
> > > >   values otherwise; there are no issues on trac about this
> > > 
> > > Are the values in the "otherwise" case maybe good enough so they
> > > worked for people with noone noticing ?
> > 
> > While working on this with a 10bit sample I was suprised this code and
> > vf_psnr showed significantly different values (IIRC not even the first
> > digit was accurate) and it took me a while to realize the scaling made
> > the assumptions it did.
> > 
> > > > * only a few encoders (namely aom, vpx, mpegvideo, snow) export this
> > > >   information; other often-used encoders such as libx26[45] do not
> > > >   export this, even though they could
> > > > 
> > > > This suggests that this feature is not useful and it is better to remove
> > > > it rather than spend effort on fixing it.
> > > > ---
> > > > I needed to resolve this code's interaction with encoders as a part of
> > > > my multithreading work and spent a few hours on it. Making it work
> > > > correctly in all cases seems nontrivial and duplicates a lot of the
> > > > logic from vf_psnr.
> > > 
> > > Can anything missing in vf_psnr be added into it and then vf_psnr used ?
> > > I agree that duplicating PSNR code and logic is bad
> > 
> > Nothing is missing in vf_psnr AFAIK, the difference is that these values
> > are produced directly by the encoder, so you don't need a
> > decoding+filtering pass to obtain the numbers.
> 
> That sounds like a missing feature.
> vf_psnr cannot use the encoder values, it always needs to recompute them.
> The encoder could export these values in metadata and vf_psnr could
> then check if the frame pairs already have their psnr computed and use
> that.

In general I agree that making those values more widely usable would be
good, but there's a number of issues that would need to be addressed
first:
* encoders export encoded packet properties, but AFAIK no muxer can
  store them
* even if a muxer could store them or encoded packets could be directly
  passed to a decoder (this should actually be possible once I'm done
  with this work), the data is only defined for packets and not frames
* possibly something like showinfo would be more suited for displaying
  this
None of these are insurmountable, but it's a fair amount of work.
Patches welcome.

> > The question more broadly is - what is this supposed to be useful for?
> > PSNR is a highly flawed metric and AFAIU state of the art moved several
> > generations away from it. And for a quick-and-dirty quality estimate,
> > the bitrate and QP might be more informative for most users, and are
> > supported by more encoders. So what is the point of printing this
> > information?
> 
> The "problem" isnt psnr specific
> having a filter which could provide another metric and be able to use
> encoder supplied data when available or compute them from encoder input +
> decoded images in a way that is automatic would be useful
> It would mean a single syntax a user could use to get the metric he wants
> while using the optimal implementation

The problem is PSNR-specific code in ffmpeg CLI that is blocking my
work. Sounds to me we are in agreement that this code should not be
there.

-- 
Anton Khirnov
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [RFC/PATCH] fftools/ffmpeg: stop printing PSNR information in status report

[Anton Khirnov]

If nobody has more comments or objections, I'm planning to push this on
Monday.

-- 
Anton Khirnov
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".