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

[Mats Peterson]

On 12/28/2015 03:32 PM, Mats Peterson wrote:
> On 12/28/2015 03:29 PM, Michael Niedermayer wrote:
>> On Mon, Dec 28, 2015 at 12:07:38PM +0100, Nicolas George wrote:
>>> L'octidi 8 nivôse, an CCXXIV, Mats Peterson a écrit :
>>>> Michael, he's talking about the OLD patch that was never applied. My
>>>> patch
>>>> has been written from scratch, more or less. I did borrowed some
>>>> palette
>>>> loops from mov.c, but I have also attributed the previous authors at
>>>> the top
>>>> of qtpalette.c properly.
>>>
>>> I must say, I find this hunk from 7973603:
>>>
>>> +        if (matroska->has_palette) {
>>> +            uint8_t *pal = av_packet_new_side_data(pkt,
>>> AV_PKT_DATA_PALETTE, AVPALETTE_SIZE);
>>> +            if (!pal) {
>>> +                av_log(matroska->ctx, AV_LOG_ERROR, "Cannot append
>>> palette to packet\n");
>>> +            } else {
>>> +                memcpy(pal, matroska->palette, AVPALETTE_SIZE);
>>> +            }
>>> +            matroska->has_palette = 0;
>>> +        }
>>>
>>> looks quite similar to this hunk from
>>> https://trac.ffmpeg.org/attachment/ticket/5071/patchmkvmov.diff :
>>>
>>> +        if (matroska->pal) {
>>> +            uint8_t *pal = av_packet_new_side_data(pkt,
>>> AV_PKT_DATA_PALETTE, AVPALETTE_SIZE);
>>> +            if (!pal) {
>>> +                av_log(matroska->ctx, AV_LOG_ERROR, "Cannot append
>>> palette to packet\n");
>>> +            } else {
>>> +                memcpy(pal, matroska->pal, AVPALETTE_SIZE);
>>> +            }
>>> +            av_freep(&matroska->pal);
>>> +        }
>>>
>>> Especially the use of if/else for error, and actually ignoring the
>>> error,
>>> instead of the most common (and more correct, but the rest of the code
>>> ignores error too) "if...return AVERROR(ENOMEM)".
>>
>> for reference: (similar code prior to the patches)
>>
>> HEAD~20:libavdevice/x11grab.c-    pkt->pts  = curtime;
>> HEAD~20:libavdevice/x11grab.c-    if (s->palette_changed) {
>> HEAD~20:libavdevice/x11grab.c-        uint8_t *pal =
>> av_packet_new_side_data(pkt, AV_PKT_DATA_PALETTE,
>> HEAD~20:libavdevice/x11grab.c-
>> AVPALETTE_SIZE);
>> HEAD~20:libavdevice/x11grab.c-        if (!pal) {
>> HEAD~20:libavdevice/x11grab.c:            av_log(s, AV_LOG_ERROR,
>> "Cannot append palette to packet\n");
>> HEAD~20:libavdevice/x11grab.c-        } else {
>> HEAD~20:libavdevice/x11grab.c-            memcpy(pal, s->palette,
>> AVPALETTE_SIZE);
>> HEAD~20:libavdevice/x11grab.c-            s->palette_changed = 0;
>> HEAD~20:libavdevice/x11grab.c-        }
>> HEAD~20:libavdevice/x11grab.c-    }
>> --
>> HEAD~20:libavformat/asfdec_f.c-            if (asf_st->pkt.data &&
>> asf_st->palette_changed) {
>> HEAD~20:libavformat/asfdec_f.c-                uint8_t *pal;
>> HEAD~20:libavformat/asfdec_f.c-                pal =
>> av_packet_new_side_data(&asf_st->pkt, AV_PKT_DATA_PALETTE,
>> HEAD~20:libavformat/asfdec_f.c-
>> AVPALETTE_SIZE);
>> HEAD~20:libavformat/asfdec_f.c-                if (!pal) {
>> HEAD~20:libavformat/asfdec_f.c:                    av_log(s,
>> AV_LOG_ERROR, "Cannot append palette to packet\n");
>> HEAD~20:libavformat/asfdec_f.c-                } else {
>> HEAD~20:libavformat/asfdec_f.c-                    memcpy(pal,
>> asf_st->palette, AVPALETTE_SIZE);
>> HEAD~20:libavformat/asfdec_f.c-
>> asf_st->palette_changed = 0;
>> HEAD~20:libavformat/asfdec_f.c-                }
>> HEAD~20:libavformat/asfdec_f.c-            }
>> --
>> HEAD~20:libavformat/mov.c-        if (sc->has_palette) {
>> HEAD~20:libavformat/mov.c-            uint8_t *pal;
>> HEAD~20:libavformat/mov.c-
>> HEAD~20:libavformat/mov.c-            pal =
>> av_packet_new_side_data(pkt, AV_PKT_DATA_PALETTE, AVPALETTE_SIZE);
>> HEAD~20:libavformat/mov.c-            if (!pal) {
>> HEAD~20:libavformat/mov.c:                av_log(mov->fc,
>> AV_LOG_ERROR, "Cannot append palette to packet\n");
>> HEAD~20:libavformat/mov.c-            } else {
>> HEAD~20:libavformat/mov.c-                memcpy(pal, sc->palette,
>> AVPALETTE_SIZE);
>> HEAD~20:libavformat/mov.c-                sc->has_palette = 0;
>> HEAD~20:libavformat/mov.c-            }
>> HEAD~20:libavformat/mov.c-        }
>>
>> [...]
>>
>>
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>
> Interesting ;)
>

By the way, here's a clip from the old patch (THIS one had significant 
parts written by Carl alright) that got rave reviews from Hendrik 
Leppkes, calling it "an extremely ugly hack", complaining at the calling 
into another demuxer (by using the ff_mov_read_stsd_entries() function 
in mov.c):


+            priv_data = st->priv_data;
+            nb_streams = s->nb_streams;
+            mc = av_mallocz(sizeof(*mc));
+            if (! mc)
+                return AVERROR(ENOMEM);
+            mc->fc = s;
+            st->priv_data = msc = av_mallocz(sizeof(MOVStreamContext));
+            if (!msc) {
+                av_free(mc);
+                st->priv_data = priv_data;
+                return AVERROR(ENOMEM);
+            }
+            ffio_init_context(&pb, track->codec_priv.data, 
track->codec_priv.size, 0, NULL, NULL, NULL, NULL);
+            /* ff_mov_read_stsd_entries updates stream s->nb_streams-1,
+             * so set it temporarily to indicate which stream to update. */
+            s->nb_streams = st->index + 1;
+            ff_mov_read_stsd_entries(mc, &pb, 1);
+

Mats

-- 
Mats Peterson
http://matsp888.no-ip.org/~mats/