[FFmpeg-devel] avcodec/amv: use correct quantize tables

The official encoder has a mode to generate mjpeg AVI files, which
embeds the correct quantize table. Change the table we use (both encoder
and decoder) to match that.

Signed-off-by: AlexGuo1998 <AlexGuo1998@163.com>
---
 libavcodec/mpegvideo_enc.c | 16 +++++++---------
 libavcodec/sp5x.h          | 21 +++++++++++++++++++++
 libavcodec/sp5xdec.c       |  9 +++++++--
 3 files changed, 35 insertions(+), 11 deletions(-)

So sorry that gmail trashed the patch. Re-sent as an attachment.
From 14910cef473a2858d5cf5e3bf855730762456de1 Mon Sep 17 00:00:00 2001
From: AlexGuo1998 <AlexGuo1998@163.com>
Date: Sun, 9 Apr 2023 15:57:05 +0800
Subject: [PATCH] avcodec/amv: use correct quantize tables

The official encoder has a mode to generate mjpeg AVI files, which
embeds the correct quantize table. Change the table we use (both encoder
and decoder) to match that.

Signed-off-by: AlexGuo1998 <AlexGuo1998@163.com>
---
 libavcodec/mpegvideo_enc.c | 16 +++++++---------
 libavcodec/sp5x.h          | 21 +++++++++++++++++++++
 libavcodec/sp5xdec.c       |  9 +++++++--
 3 files changed, 35 insertions(+), 11 deletions(-)

diff --git a/libavcodec/mpegvideo_enc.c b/libavcodec/mpegvideo_enc.c
index 7d3c8875f2..5592095ed2 100644
--- a/libavcodec/mpegvideo_enc.c
+++ b/libavcodec/mpegvideo_enc.c
@@ -2878,9 +2878,9 @@ static int encode_thread(AVCodecContext *c, void *arg){
         s->encoding_error[i] = 0;
     }
     if(s->codec_id==AV_CODEC_ID_AMV){
-        s->last_dc[0] = 128*8/13;
-        s->last_dc[1] = 128*8/14;
-        s->last_dc[2] = 128*8/14;
+        s->last_dc[0] = 128*8/8;
+        s->last_dc[1] = 128*8/9;
+        s->last_dc[2] = 128*8/9;
     }
     s->mb_skip_run = 0;
     memset(s->last_mv, 0, sizeof(s->last_mv));
@@ -3755,18 +3755,16 @@ static int encode_picture(MpegEncContext *s)
         s->qscale= 8;
 
         if (s->codec_id == AV_CODEC_ID_AMV) {
-            static const uint8_t y[32] = {13,13,13,13,13,13,13,13,13,13,13,13,13,13,13,13,13,13,13,13,13,13,13,13,13,13,13,13,13,13,13,13};
-            static const uint8_t c[32] = {14,14,14,14,14,14,14,14,14,14,14,14,14,14,14,14,14,14,14,14,14,14,14,14,14,14,14,14,14,14,14,14};
+            static const uint8_t y[32] = {8,8,8,8,8,8,8,8,8,8,8,8,8,8,8,8,8,8,8,8,8,8,8,8,8,8,8,8,8,8,8,8};
+            static const uint8_t c[32] = {9,9,9,9,9,9,9,9,9,9,9,9,9,9,9,9,9,9,9,9,9,9,9,9,9,9,9,9,9,9,9,9};
             for (int i = 1; i < 64; i++) {
                 int j = s->idsp.idct_permutation[ff_zigzag_direct[i]];
 
-                s->intra_matrix[j]        = sp5x_qscale_five_quant_table[0][i];
-                s->chroma_intra_matrix[j] = sp5x_qscale_five_quant_table[1][i];
+                s->intra_matrix[j]        = amv_quant_table[0][i];
+                s->chroma_intra_matrix[j] = amv_quant_table[1][i];
             }
             s->y_dc_scale_table = y;
             s->c_dc_scale_table = c;
-            s->intra_matrix[0] = 13;
-            s->chroma_intra_matrix[0] = 14;
             ff_convert_matrix(s, s->q_intra_matrix, s->q_intra_matrix16,
                               s->intra_matrix, s->intra_quant_bias, 8, 8, 1);
             ff_convert_matrix(s, s->q_chroma_intra_matrix, s->q_chroma_intra_matrix16,
diff --git a/libavcodec/sp5x.h b/libavcodec/sp5x.h
index d84d851768..4646914235 100644
--- a/libavcodec/sp5x.h
+++ b/libavcodec/sp5x.h
@@ -145,4 +145,25 @@ static const uint8_t sp5x_qscale_five_quant_table[][64]=
        79, 79, 79, 79, 79, 79, 79, 79, 79, 79, 79, 79, 79, 79, 79, 79 },
 };
 
+static const uint8_t amv_quant_table[][64] =
+{
+    /* AMV: always constant quantize table */
+    {   0x08, 0x06, 0x06, 0x07, 0x06, 0x05, 0x08, 0x07,
+        0x07, 0x07, 0x09, 0x09, 0x08, 0x0A, 0x0C, 0x14,
+        0x0D, 0x0C, 0x0B, 0x0B, 0x0C, 0x19, 0x12, 0x13,
+        0x0F, 0x14, 0x1D, 0x1A, 0x1F, 0x1E, 0x1D, 0x1A,
+        0x1C, 0x1C, 0x20, 0x24, 0x2E, 0x27, 0x20, 0x22,
+        0x2C, 0x23, 0x1C, 0x1C, 0x28, 0x37, 0x29, 0x2C,
+        0x30, 0x31, 0x34, 0x34, 0x34, 0x1F, 0x27, 0x39,
+        0x3D, 0x38, 0x32, 0x3C, 0x2E, 0x33, 0x34, 0x32 },
+    {   0x09, 0x09, 0x09, 0x0C, 0x0B, 0x0C, 0x18, 0x0D,
+        0x0D, 0x18, 0x32, 0x21, 0x1C, 0x21, 0x32, 0x32,
+        0x32, 0x32, 0x32, 0x32, 0x32, 0x32, 0x32, 0x32,
+        0x32, 0x32, 0x32, 0x32, 0x32, 0x32, 0x32, 0x32,
+        0x32, 0x32, 0x32, 0x32, 0x32, 0x32, 0x32, 0x32,
+        0x32, 0x32, 0x32, 0x32, 0x32, 0x32, 0x32, 0x32,
+        0x32, 0x32, 0x32, 0x32, 0x32, 0x32, 0x32, 0x32,
+        0x32, 0x32, 0x32, 0x32, 0x32, 0x32, 0x32, 0x32 },
+};
+
 #endif /* AVCODEC_SP5X_H */
diff --git a/libavcodec/sp5xdec.c b/libavcodec/sp5xdec.c
index dfed725500..8c8b4f67fa 100644
--- a/libavcodec/sp5xdec.c
+++ b/libavcodec/sp5xdec.c
@@ -55,8 +55,13 @@ static int sp5x_decode_frame(AVCodecContext *avctx,
     recoded[j++] = 0xD8;
 
     memcpy(recoded+j, &sp5x_data_dqt[0], sizeof(sp5x_data_dqt));
-    memcpy(recoded + j + 5,  &sp5x_qscale_five_quant_table[0], 64);
-    memcpy(recoded + j + 70, &sp5x_qscale_five_quant_table[1], 64);
+    if (avctx->codec_id==AV_CODEC_ID_AMV) {
+        memcpy(recoded + j + 5,  &amv_quant_table[0], 64);
+        memcpy(recoded + j + 70, &amv_quant_table[1], 64);
+    } else {
+        memcpy(recoded + j + 5,  &sp5x_qscale_five_quant_table[0], 64);
+        memcpy(recoded + j + 70, &sp5x_qscale_five_quant_table[1], 64);
+    }
     j += sizeof(sp5x_data_dqt);
 
     memcpy(recoded+j, &sp5x_data_dht[0], sizeof(sp5x_data_dht));

On Sun, Apr 09, 2023 at 04:37:49PM +0800, AlexGuo1998 wrote:
> So sorry that gmail trashed the patch. Re-sent as an attachment.

> From 14910cef473a2858d5cf5e3bf855730762456de1 Mon Sep 17 00:00:00 2001
> From: AlexGuo1998 <AlexGuo1998@163.com>
> Date: Sun, 9 Apr 2023 15:57:05 +0800
> Subject: [PATCH] avcodec/amv: use correct quantize tables
> 
> The official encoder has a mode to generate mjpeg AVI files, which
> embeds the correct quantize table. Change the table we use (both encoder
> and decoder) to match that.
> 
> Signed-off-by: AlexGuo1998 <AlexGuo1998@163.com>
> ---
>  libavcodec/mpegvideo_enc.c | 16 +++++++---------
>  libavcodec/sp5x.h          | 21 +++++++++++++++++++++
>  libavcodec/sp5xdec.c       |  9 +++++++--
>  3 files changed, 35 insertions(+), 11 deletions(-)

decoder and encoder changes should be in seperate patches
and the version should be increased for encoder changes so
any software reading a file can from a bitstream version metadata
indentify it. For this to work of course the version first needs to be
stored. So one should check LIBAVCODEC_IDENT is stored

This would then also allow the decoder to match the encoder optimally

thx

[...]

On Mon, Apr 10, 2023 at 7:43 AM Michael Niedermayer wrote:
>
> On Sun, Apr 09, 2023 at 04:37:49PM +0800, AlexGuo1998 wrote:
> > So sorry that gmail trashed the patch. Re-sent as an attachment.
>
> > From 14910cef473a2858d5cf5e3bf855730762456de1 Mon Sep 17 00:00:00 2001
> > From: AlexGuo1998 <AlexGuo1998@163.com>
> > Date: Sun, 9 Apr 2023 15:57:05 +0800
> > Subject: [PATCH] avcodec/amv: use correct quantize tables
> >
> > The official encoder has a mode to generate mjpeg AVI files, which
> > embeds the correct quantize table. Change the table we use (both encoder
> > and decoder) to match that.
> >
> > Signed-off-by: AlexGuo1998 <AlexGuo1998@163.com>
> > ---
> >  libavcodec/mpegvideo_enc.c | 16 +++++++---------
> >  libavcodec/sp5x.h          | 21 +++++++++++++++++++++
> >  libavcodec/sp5xdec.c       |  9 +++++++--
> >  3 files changed, 35 insertions(+), 11 deletions(-)
>
> decoder and encoder changes should be in seperate patches

I'll do that later.

> and the version should be increased for encoder changes so
> any software reading a file can from a bitstream version metadata
> indentify it. For this to work of course the version first needs to be
> stored. So one should check LIBAVCODEC_IDENT is stored
>
> This would then also allow the decoder to match the encoder optimally

I'd like to, but unsure if it is possible. As noted in commit a2fea0f4,
"AMV is a hard-coded (and broken) subset of AVI", it's intended to be
played on a function limited hardware device.

The container is a AVI like container, with some fields intentionally
zeroed. Some player won't play when those fields are set correctly.

The video stream is a nonstandard "mjpeg like" data stream, with
compressed data stream directly after SOI marker.

Those left us with nowhere for additional metadata. Should I introduce
an AVOption to restore the old (possibly incorrect) behavior?

On Mon, Apr 10, 2023 at 12:09:59PM +0800, AlexGuo1998 wrote:
> On Mon, Apr 10, 2023 at 7:43 AM Michael Niedermayer wrote:
> >
> > On Sun, Apr 09, 2023 at 04:37:49PM +0800, AlexGuo1998 wrote:
> > > So sorry that gmail trashed the patch. Re-sent as an attachment.
> >
> > > From 14910cef473a2858d5cf5e3bf855730762456de1 Mon Sep 17 00:00:00 2001
> > > From: AlexGuo1998 <AlexGuo1998@163.com>
> > > Date: Sun, 9 Apr 2023 15:57:05 +0800
> > > Subject: [PATCH] avcodec/amv: use correct quantize tables
> > >
> > > The official encoder has a mode to generate mjpeg AVI files, which
> > > embeds the correct quantize table. Change the table we use (both encoder
> > > and decoder) to match that.
> > >
> > > Signed-off-by: AlexGuo1998 <AlexGuo1998@163.com>
> > > ---
> > >  libavcodec/mpegvideo_enc.c | 16 +++++++---------
> > >  libavcodec/sp5x.h          | 21 +++++++++++++++++++++
> > >  libavcodec/sp5xdec.c       |  9 +++++++--
> > >  3 files changed, 35 insertions(+), 11 deletions(-)
> >
> > decoder and encoder changes should be in seperate patches
> 
> I'll do that later.
> 
> > and the version should be increased for encoder changes so
> > any software reading a file can from a bitstream version metadata
> > indentify it. For this to work of course the version first needs to be
> > stored. So one should check LIBAVCODEC_IDENT is stored
> >
> > This would then also allow the decoder to match the encoder optimally
> 
> I'd like to, but unsure if it is possible. As noted in commit a2fea0f4,
> "AMV is a hard-coded (and broken) subset of AVI", it's intended to be
> played on a function limited hardware device.
> 
> The container is a AVI like container, with some fields intentionally
> zeroed. Some player won't play when those fields are set correctly.
> 
> The video stream is a nonstandard "mjpeg like" data stream, with
> compressed data stream directly after SOI marker.
> 

> Those left us with nowhere for additional metadata. Should I introduce

well, there are places left, 

if its worth the effort, iam not sure

For example the huffman coder has a EOB code and a 16 zeros code
In blocks with more than 16 zeros left at the end teh 16 zero code
can be placed before teh EOB. A working decoder needs to support both
the 16 zero and EOB codes so this should not break anything, its just
a less efficient way to store a block.
There are other places like at the end of frames where decoder may
or may not have an issue with some extra chunk

Thats more a [RFC] than a request to do this.
Is there anything that can be used to distingish what ffmpeg generated
previously (well currently) and what the official software did create?

If not i guess this is not worth it as we already have indistingishable
cases then


> an AVOption to restore the old (possibly incorrect) behavior?

does the old encoder output look better with the old decoder ?

thx

[...]