diff mbox series

[FFmpeg-devel,37/57] avcodec/mpegutils: Don't output wrong mb skip values

Message ID GV1P250MB07376F0A990BB561544C42838FC02@GV1P250MB0737.EURP250.PROD.OUTLOOK.COM
State New
Headers show
Series [FFmpeg-devel,01/57] avcodec/vc1: Combine identical checks | expand

Commit Message

Andreas Rheinhardt June 12, 2024, 1:48 p.m. UTC
The earlier code had two problems:
1. For reference frames that are not directly output (happens unless
low_delay is set), the mb skip values referred to the next reference
frame to be decoded.
2. For non-reference frames, every macroblock was always considered
skipped.
This makes the output (worse than) useless; that no one ever
complained about this shows that this feature is not really used.
It is therefore removed.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/h264dec.c       |  2 +-
 libavcodec/mpegutils.c     | 12 ++----------
 libavcodec/mpegutils.h     |  2 +-
 libavcodec/mpegvideo_dec.c |  2 +-
 4 files changed, 5 insertions(+), 13 deletions(-)

Comments

Michael Niedermayer June 13, 2024, 7:27 a.m. UTC | #1
On Wed, Jun 12, 2024 at 03:48:33PM +0200, Andreas Rheinhardt wrote:
> The earlier code had two problems:
> 1. For reference frames that are not directly output (happens unless
> low_delay is set), the mb skip values referred to the next reference
> frame to be decoded.
> 2. For non-reference frames, every macroblock was always considered
> skipped.
> This makes the output (worse than) useless; that no one ever
> complained about this shows that this feature is not really used.
> It is therefore removed.

I used it for statistical purposes long ago, seeing how much blocks where
skiped and for that a +-1 frame difference is inconsequantal. Also
i understood that B frame are handled differently, its an internal
bitstream related number after all

thx

[...]
Andreas Rheinhardt June 17, 2024, 11:08 a.m. UTC | #2
Michael Niedermayer:
> On Wed, Jun 12, 2024 at 03:48:33PM +0200, Andreas Rheinhardt wrote:
>> The earlier code had two problems:
>> 1. For reference frames that are not directly output (happens unless
>> low_delay is set), the mb skip values referred to the next reference
>> frame to be decoded.
>> 2. For non-reference frames, every macroblock was always considered
>> skipped.
>> This makes the output (worse than) useless; that no one ever
>> complained about this shows that this feature is not really used.
>> It is therefore removed.
> 
> I used it for statistical purposes long ago, seeing how much blocks where
> skiped and for that a +-1 frame difference is inconsequantal. Also
> i understood that B frame are handled differently, its an internal
> bitstream related number after all

Does "long ago" mean that you agree that it is unused and can be removed?

- Andreas
Michael Niedermayer June 18, 2024, 10:08 p.m. UTC | #3
On Mon, Jun 17, 2024 at 01:08:56PM +0200, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > On Wed, Jun 12, 2024 at 03:48:33PM +0200, Andreas Rheinhardt wrote:
> >> The earlier code had two problems:
> >> 1. For reference frames that are not directly output (happens unless
> >> low_delay is set), the mb skip values referred to the next reference
> >> frame to be decoded.
> >> 2. For non-reference frames, every macroblock was always considered
> >> skipped.
> >> This makes the output (worse than) useless; that no one ever
> >> complained about this shows that this feature is not really used.
> >> It is therefore removed.
> > 
> > I used it for statistical purposes long ago, seeing how much blocks where
> > skiped and for that a +-1 frame difference is inconsequantal. Also
> > i understood that B frame are handled differently, its an internal
> > bitstream related number after all
> 
> Does "long ago" mean that you agree that it is unused and can be removed?

It simply means that it was usefull and a number similar could be usefull
again. I dont care much about it, judging from the past i might use this once
in 10 years, if its available. If its not available its not.

thx

[...]
Andreas Rheinhardt June 19, 2024, 1:54 p.m. UTC | #4
Michael Niedermayer:
> On Mon, Jun 17, 2024 at 01:08:56PM +0200, Andreas Rheinhardt wrote:
>> Michael Niedermayer:
>>> On Wed, Jun 12, 2024 at 03:48:33PM +0200, Andreas Rheinhardt wrote:
>>>> The earlier code had two problems:
>>>> 1. For reference frames that are not directly output (happens unless
>>>> low_delay is set), the mb skip values referred to the next reference
>>>> frame to be decoded.
>>>> 2. For non-reference frames, every macroblock was always considered
>>>> skipped.
>>>> This makes the output (worse than) useless; that no one ever
>>>> complained about this shows that this feature is not really used.
>>>> It is therefore removed.
>>>
>>> I used it for statistical purposes long ago, seeing how much blocks where
>>> skiped and for that a +-1 frame difference is inconsequantal. Also
>>> i understood that B frame are handled differently, its an internal
>>> bitstream related number after all
>>
>> Does "long ago" mean that you agree that it is unused and can be removed?
> 
> It simply means that it was usefull and a number similar could be usefull
> again. I dont care much about it, judging from the past i might use this once
> in 10 years, if its available. If its not available its not.
> 

Ok, then I'll apply this patchset tomorrow (unless there are any
objections by anyone or unless you (or anyone) wants more time to look
at it).

- Andreas
diff mbox series

Patch

diff --git a/libavcodec/h264dec.c b/libavcodec/h264dec.c
index fd23e367b4..c77d8f42db 100644
--- a/libavcodec/h264dec.c
+++ b/libavcodec/h264dec.c
@@ -979,7 +979,7 @@  static int finalize_frame(H264Context *h, AVFrame *dst, H264Picture *out, int *g
         *got_frame = 1;
 
         if (CONFIG_MPEGVIDEODEC) {
-            ff_print_debug_info2(h->avctx, dst, NULL,
+            ff_print_debug_info2(h->avctx, dst,
                                  out->mb_type,
                                  out->qscale_table,
                                  out->motion_val,
diff --git a/libavcodec/mpegutils.c b/libavcodec/mpegutils.c
index a53996852f..73b6650b70 100644
--- a/libavcodec/mpegutils.c
+++ b/libavcodec/mpegutils.c
@@ -153,7 +153,7 @@  static char get_interlacement_char(int mb_type)
 }
 
 void ff_print_debug_info2(AVCodecContext *avctx, AVFrame *pict,
-                          const uint8_t *mbskip_table, const uint32_t *mbtype_table,
+                          const uint32_t *mbtype_table,
                           const int8_t *qscale_table, int16_t (*const motion_val[2])[2],
                           int mb_width, int mb_height, int mb_stride, int quarter_sample)
 {
@@ -248,7 +248,7 @@  void ff_print_debug_info2(AVCodecContext *avctx, AVFrame *pict,
         return;
 
 
-    if (avctx->debug & (FF_DEBUG_SKIP | FF_DEBUG_QP | FF_DEBUG_MB_TYPE)) {
+    if (avctx->debug & (FF_DEBUG_QP | FF_DEBUG_MB_TYPE)) {
         int x,y;
         AVBPrint buf;
         int n;
@@ -267,8 +267,6 @@  void ff_print_debug_info2(AVCodecContext *avctx, AVFrame *pict,
         av_bprint_chars(&buf, ' ', margin_left);
 
         n = 0;
-        if (avctx->debug & FF_DEBUG_SKIP)
-            n++;
         if (avctx->debug & FF_DEBUG_QP)
             n += 2;
         if (avctx->debug & FF_DEBUG_MB_TYPE)
@@ -284,12 +282,6 @@  void ff_print_debug_info2(AVCodecContext *avctx, AVFrame *pict,
             for (x = 0; x < mb_width; x++) {
                 if (x == 0)
                     av_bprintf(&buf, "%*d ", margin_left - 1, y << 4);
-                if (avctx->debug & FF_DEBUG_SKIP) {
-                    int count = mbskip_table ? mbskip_table[x + y * mb_stride] : 0;
-                    if (count > 9)
-                        count = 9;
-                    av_bprintf(&buf, "%1d", count);
-                }
                 if (avctx->debug & FF_DEBUG_QP) {
                     av_bprintf(&buf, "%2d", qscale_table[x + y * mb_stride]);
                 }
diff --git a/libavcodec/mpegutils.h b/libavcodec/mpegutils.h
index 43075191c6..64e69c7746 100644
--- a/libavcodec/mpegutils.h
+++ b/libavcodec/mpegutils.h
@@ -106,7 +106,7 @@  void ff_draw_horiz_band(AVCodecContext *avctx, const AVFrame *cur, const AVFrame
  * Print debugging info for the given picture.
  */
 void ff_print_debug_info2(AVCodecContext *avctx, AVFrame *pict,
-                          const uint8_t *mbskip_table, const uint32_t *mbtype_table,
+                          const uint32_t *mbtype_table,
                           const int8_t *qscale_table, int16_t (*const motion_val[2])[2],
                           int mb_width, int mb_height, int mb_stride, int quarter_sample);
 
diff --git a/libavcodec/mpegvideo_dec.c b/libavcodec/mpegvideo_dec.c
index e95b5a0940..4e279d9fa8 100644
--- a/libavcodec/mpegvideo_dec.c
+++ b/libavcodec/mpegvideo_dec.c
@@ -408,7 +408,7 @@  void ff_mpv_frame_end(MpegEncContext *s)
 
 void ff_print_debug_info(const MpegEncContext *s, const MPVPicture *p, AVFrame *pict)
 {
-    ff_print_debug_info2(s->avctx, pict, s->mbskip_table, p->mb_type,
+    ff_print_debug_info2(s->avctx, pict, p->mb_type,
                          p->qscale_table, p->motion_val,
                          s->mb_width, s->mb_height, s->mb_stride, s->quarter_sample);
 }