diff mbox

[FFmpeg-devel,1/4] avformat/qtpalette: parse color table according to the QuickTime file format specs

Message ID 20180419193221.21712-1-cus@passwd.hu
State Accepted
Commit c60a824ee87ae3b15ed1cb92b780bec9b642b019
Headers show

Commit Message

Marton Balint April 19, 2018, 7:32 p.m. UTC
The specs says that the the first color component in the color array is
not alpha, but simply 0.

Fixes 0 alpha of fate-suite/cvid/catfight-cvid-pal8-partial.mov

Signed-off-by: Marton Balint <cus@passwd.hu>
---
 libavformat/qtpalette.c             | 12 ++++++------
 tests/ref/lavf-fate/mov_qtrle_mace6 |  2 +-
 2 files changed, 7 insertions(+), 7 deletions(-)

Comments

Michael Niedermayer April 19, 2018, 11:21 p.m. UTC | #1
On Thu, Apr 19, 2018 at 09:32:18PM +0200, Marton Balint wrote:
> The specs says that the the first color component in the color array is
> not alpha, but simply 0.
> 
> Fixes 0 alpha of fate-suite/cvid/catfight-cvid-pal8-partial.mov
> 
> Signed-off-by: Marton Balint <cus@passwd.hu>
> ---
>  libavformat/qtpalette.c             | 12 ++++++------
>  tests/ref/lavf-fate/mov_qtrle_mace6 |  2 +-
>  2 files changed, 7 insertions(+), 7 deletions(-)

this breaks libavcodec/tests/imgconvert.c

@@ -10,6 +10,7 @@
 pix fmt monow yuv_plan:0 avg_bpp:1
 pix fmt monob yuv_plan:0 avg_bpp:1
 pix fmt pal8 yuv_plan:0 avg_bpp:8
+Alpha flag mismatch
 pix fmt yuvj420p yuv_plan:1 avg_bpp:12
 pix fmt yuvj422p yuv_plan:1 avg_bpp:16
 pix fmt yuvj444p yuv_plan:1 avg_bpp:24


[...]
Michael Niedermayer April 19, 2018, 11:23 p.m. UTC | #2
On Fri, Apr 20, 2018 at 01:21:15AM +0200, Michael Niedermayer wrote:
> On Thu, Apr 19, 2018 at 09:32:18PM +0200, Marton Balint wrote:
> > The specs says that the the first color component in the color array is
> > not alpha, but simply 0.
> > 
> > Fixes 0 alpha of fate-suite/cvid/catfight-cvid-pal8-partial.mov
> > 
> > Signed-off-by: Marton Balint <cus@passwd.hu>
> > ---
> >  libavformat/qtpalette.c             | 12 ++++++------
> >  tests/ref/lavf-fate/mov_qtrle_mace6 |  2 +-
> >  2 files changed, 7 insertions(+), 7 deletions(-)
> 
> this breaks libavcodec/tests/imgconvert.c

replied to the wrong patch, this should have been for
"avfilter/avfiltergraph: fix has_alpha in pick_format"


> 
> @@ -10,6 +10,7 @@
>  pix fmt monow yuv_plan:0 avg_bpp:1
>  pix fmt monob yuv_plan:0 avg_bpp:1
>  pix fmt pal8 yuv_plan:0 avg_bpp:8
> +Alpha flag mismatch
>  pix fmt yuvj420p yuv_plan:1 avg_bpp:12
>  pix fmt yuvj422p yuv_plan:1 avg_bpp:16
>  pix fmt yuvj444p yuv_plan:1 avg_bpp:24

[...]
Michael Niedermayer April 19, 2018, 11:51 p.m. UTC | #3
On Thu, Apr 19, 2018 at 09:32:18PM +0200, Marton Balint wrote:
> The specs says that the the first color component in the color array is
> not alpha, but simply 0.
> 
> Fixes 0 alpha of fate-suite/cvid/catfight-cvid-pal8-partial.mov
> 
> Signed-off-by: Marton Balint <cus@passwd.hu>
> ---
>  libavformat/qtpalette.c             | 12 ++++++------
>  tests/ref/lavf-fate/mov_qtrle_mace6 |  2 +-
>  2 files changed, 7 insertions(+), 7 deletions(-)

no objections to this (assuming this works with all files)
i think its probably best to wait a bit before pushing this
so people who are interrested in this can check the files they have
(it seems to work with what i tested it with but thats not every file)

[...]
diff mbox

Patch

diff --git a/libavformat/qtpalette.c b/libavformat/qtpalette.c
index 666c6b7351..6833f0cea9 100644
--- a/libavformat/qtpalette.c
+++ b/libavformat/qtpalette.c
@@ -49,7 +49,7 @@  int ff_get_qtpalette(int codec_id, AVIOContext *pb, uint32_t *palette)
     /* If the depth is 1, 2, 4, or 8 bpp, file is palettized. */
     if ((bit_depth == 1 || bit_depth == 2 || bit_depth == 4 || bit_depth == 8)) {
         uint32_t color_count, color_start, color_end;
-        uint32_t a, r, g, b;
+        uint32_t r, g, b;
 
         /* Ignore the greyscale bit for 1-bit video and sample
          * descriptions containing a color table. */
@@ -94,17 +94,17 @@  int ff_get_qtpalette(int codec_id, AVIOContext *pb, uint32_t *palette)
             color_end = avio_rb16(pb);
             if ((color_start <= 255) && (color_end <= 255)) {
                 for (i = color_start; i <= color_end; i++) {
-                    /* each A, R, G, or B component is 16 bits;
-                     * only use the top 8 bits */
-                    a = avio_r8(pb);
-                    avio_r8(pb);
+                    /* Each color is made of four unsigned 16 bit integers. The
+                     * first integer is 0, the remaining integers are the red,
+                     * the green and the blue values. We only use the top 8 bit. */
+                    avio_skip(pb, 2);
                     r = avio_r8(pb);
                     avio_r8(pb);
                     g = avio_r8(pb);
                     avio_r8(pb);
                     b = avio_r8(pb);
                     avio_r8(pb);
-                    palette[i] = (a << 24 ) | (r << 16) | (g << 8) | (b);
+                    palette[i] = (0xFFU << 24) | (r << 16) | (g << 8) | (b);
                 }
             }
         }
diff --git a/tests/ref/lavf-fate/mov_qtrle_mace6 b/tests/ref/lavf-fate/mov_qtrle_mace6
index 30c705ee4c..f8428aaa49 100644
--- a/tests/ref/lavf-fate/mov_qtrle_mace6
+++ b/tests/ref/lavf-fate/mov_qtrle_mace6
@@ -1,3 +1,3 @@ 
 dcc9c4c182a5809dee9a9366f4533797 *./tests/data/lavf-fate/lavf.mov
 1270387 ./tests/data/lavf-fate/lavf.mov
-./tests/data/lavf-fate/lavf.mov CRC=0x5ec66f68
+./tests/data/lavf-fate/lavf.mov CRC=0x9320cd26