| Message ID | 20230419194832.757-1-anton@khirnov.net |
|---|---|
| State | Accepted |
| Commit | 87b4453ec6df4de78a9ede6abccdfefe7cfeb66a |
| Headers | show |
| Series | [FFmpeg-devel,RFC/PATCH] fftools/ffmpeg: stop printing PSNR information in status report | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
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? [...]
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?
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 [...]
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.
If nobody has more comments or objections, I'm planning to push this on Monday.
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 */