[FFmpeg-devel] [PATCH v8] lavf: palettized QuickTime video in Matroska

2015-12-27 Thread Mats Peterson 

Forgot to remove an av_free(). Patch explanation follows:

Palettized QuickTime video in Matroska has hitherto not been recognized
whatsoever, and the "palette" used has been completely random.

The patch for matroskadec.c fixes this issue by adding a palette side
data packet in matroska_deliver_packet(), much in the same way as it's
done in mov.c.

The change to mov.c consists mainly of moving the palette handling from
the mov_parse_stsd_video() function to a new ff_get_qtpalette() function
in the new file qtpalette.c, which is shared by both matroskadec.c and
mov.c.

Since the ff_get_qtpalette() function has to parse the video sample
description in two different ways, i.e. 1) by using the in-memory
private data when called from matroskadec.c, and 2) by reading from the
file when called from mov.c, it has to use a separate variable for each
of these sources. It may seem slightly redundant, but it is
unfortunately necessary. I would prefer that nobody touches what I've
been doing, since it's working fine right now, and it's very easy to
break things if you try to "improve it". Believe me, I've been there.

--
Mats Peterson
http://matsp888.no-ip.org/~mats/
>From c872c430d93cc450255306f52256fcbf808c3248 Mon Sep 17 00:00:00 2001
From: Mats Peterson 
Date: Sun, 27 Dec 2015 11:39:21 +0100
Subject: [PATCH v8] lavf: palettized QuickTime video in Matroska

Forgot to remove an av_free(). Patch explanation follows:

Palettized QuickTime video in Matroska has hitherto not been recognized
whatsoever, and the "palette" used has been completely random.

The patch for matroskadec.c fixes this issue by adding a palette side
data packet in matroska_deliver_packet(), much in the same way as it's
done in mov.c.

The change to mov.c consists mainly of moving the palette handling from
the mov_parse_stsd_video() function to a new ff_get_qtpalette() function
in the new file qtpalette.c, which is shared by both matroskadec.c and
mov.c.

Since the ff_get_qtpalette() function has to parse the video sample
description in two different ways, i.e. 1) by using the in-memory
private data when called from matroskadec.c, and 2) by reading from the
file when called from mov.c, it has to use a separate variable for each
of these sources. It may seem slightly redundant, but it is
unfortunately necessary. I would prefer that nobody touches what I've
been doing, since it's working fine right now, and it's very easy to
break things if you try to "improve it". Believe me, I've been there.

In matroskadec.c, I'm also putting the palette in 'extradata', like it's
done for V_MS/VFW/FOURCC; this is a requirement in order for MPlayer to
use the correct palette. And why is that, you may wonder. Well, it's
because for some mysterious reason, MPlayer adds *another* palette side
data packet *after* the one added in matroskadec.c. It uses whatever is
in extradata as the palette when adding this packet.

Video samples for testing are available at
https://drive.google.com/open?id=0B3_pEBoLs0faWElmM2FnLTZYNlk.

---
 libavformat/Makefile  |1 +
 libavformat/matroskadec.c |   30 ++-
 libavformat/mov.c |   80 
 libavformat/qtpalette.c   |  128 +
 libavformat/qtpalette.h   |6 ++-
 5 files changed, 173 insertions(+), 72 deletions(-)
 create mode 100644 libavformat/qtpalette.c

diff --git a/libavformat/Makefile b/libavformat/Makefile
index 110e9e3..e03c73e 100644
--- a/libavformat/Makefile
+++ b/libavformat/Makefile
@@ -18,6 +18,7 @@ OBJS = allformats.o \
mux.o\
options.o\
os_support.o \
+   qtpalette.o  \
riff.o   \
sdp.o\
url.o\
diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index c574749..cc3cdd0 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -64,6 +64,8 @@
 #include 
 #endif
 
+#include "qtpalette.h"
+
 typedef enum {
 EBML_NONE,
 EBML_UINT,
@@ -312,6 +314,9 @@ typedef struct MatroskaDemuxContext {
 
 /* WebM DASH Manifest live flag/ */
 int is_live;
+
+uint32_t palette[AVPALETTE_COUNT];
+int has_palette;
 } MatroskaDemuxContext;
 
 typedef struct MatroskaBlock {
@@ -1856,7 +1861,7 @@ static int matroska_parse_tracks(AVFormatContext *s)
 fourcc   = st->codec->codec_tag;
 extradata_offset = FFMIN(track->codec_priv.size, 18);
 } else if (!strcmp(track->codec_id, "A_QUICKTIME")
-   && (track->codec_priv.size >= 86)
+   && (track->codec_priv.size >= 36)
&& (track->codec_priv.data)) {
 fourcc = AV_RL32(track->codec_priv.data + 4);
 codec_id = ff_codec_get_id(ff_codec_movaudio_tags, fourcc);
@@ -1881,6 +1886,20 @@ static int matroska_parse_tracks(AVFormatContext *s)
 av_log(matroska->ctx, AV_LOG_ERROR,

Re: [FFmpeg-devel] [PATCH v8] lavf: palettized QuickTime video in Matroska

2015-12-27 Thread Hendrik Leppkes 
On Sun, Dec 27, 2015 at 11:44 AM, Mats Peterson
 wrote:
>
> Since the ff_get_qtpalette() function has to parse the video sample
> description in two different ways, i.e. 1) by using the in-memory
> private data when called from matroskadec.c, and 2) by reading from the
> file when called from mov.c, it has to use a separate variable for each
> of these sources. It may seem slightly redundant, but it is
> unfortunately necessary. I would prefer that nobody touches what I've
> been doing, since it's working fine right now, and it's very easy to
> break things if you try to "improve it". Believe me, I've been there.
>

Just to make it perfectly clear that there are no misunderstandings:
A shared utility function should offer one clear calling interface,
not two just because its convenient. Anything else is a maintenance
nightmare.

Patch not acceptable unless this is corrected.

- Hendrik
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH v8] lavf: palettized QuickTime video in Matroska

2015-12-27 Thread Mats Peterson 

On 12/27/2015 12:10 PM, Hendrik Leppkes wrote:

On Sun, Dec 27, 2015 at 11:44 AM, Mats Peterson
 wrote:


Since the ff_get_qtpalette() function has to parse the video sample
description in two different ways, i.e. 1) by using the in-memory
private data when called from matroskadec.c, and 2) by reading from the
file when called from mov.c, it has to use a separate variable for each
of these sources. It may seem slightly redundant, but it is
unfortunately necessary. I would prefer that nobody touches what I've
been doing, since it's working fine right now, and it's very easy to
break things if you try to "improve it". Believe me, I've been there.



Just to make it perfectly clear that there are no misunderstandings:
A shared utility function should offer one clear calling interface,
not two just because its convenient. Anything else is a maintenance
nightmare.

Patch not acceptable unless this is corrected.

- Hendrik
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel



You're not the maintainer, once again.

Mats

--
Mats Peterson
http://matsp888.no-ip.org/~mats/
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel