[FFmpeg-devel] [RFC] WC3 decoder without AVPaletteControl

[Reimar Döffinger]

On Wed, Apr 15, 2009 at 11:42:55AM -0700, Baptiste Coudurier wrote:
> On 4/15/2009 11:18 AM, Reimar D?ffinger wrote:
> > On Wed, Apr 15, 2009 at 10:54:01AM -0700, Baptiste Coudurier wrote:
> >> I'm pretty this av_grow_packet ugliness is useless.
> > 
> > It is needed because the formats were never designed for split demuxer
> > and decoder.
> > Without this you get stuff like in ipmovie:
> >> av_new_packet(pkt, s->decode_map_chunk_size + s->video_chunk_size)
> >> url_fseek(pb, s->decode_map_chunk_offset, SEEK_SET);
> >> get_buffer(pb, pkt->data, s->decode_map_chunk_size);
> >> url_fseek(pb, s->video_chunk_offset, SEEK_SET);
> >> get_buffer(pb, pkt->data + s->decode_map_chunk_size, s->video_chunk_size)
> > 
> > Or in most cases simply an  incomplete and sometimes subtly
> > broken but at least inconsistent ad-hoc reimplementation of it.
> > Suggestions of better solutions are of course welcome, I already asked
> > for them. Twice.
> 
> Well I already gave one: use extradata if possible.

There is no palette at all involved here.

> >>>> Also AVPacket could have ->palette_data field which seems a lot cleaner
> >>>> to me and Ronald.
> >>> Sure it is because you are completely ignoring the cases where it
> >>> doesn't work (muxing to non-AVI always, and either you have to decode
> >>> the palette etc. in the demuxer or it won't even work for remuxing into
> >>> AVI - muxing for almost all these formats in the only way to get an
> >>> index and thus quick and reliable seeking btw.).
> >> Please enumerate the formats and problems it might cause.
> >> Muxing to .mov is pretty much different than .mkv and stream copy case
> > 
> > Well, maybe I am clueless, but I see nothing to enumerate, the issue is
> > simply "where the heck do you store the data in palette_data when
> > muxing?".
> 
> Well, IMHO you need to enumarate which codecs use palette and how do
> they store this information. After that you check how palette is
> supposed to be stored in the destination container, because containers
> may have a standard way to store palette.
> 
> You can then decide how it seems usual and simple to store the palette.

I don't really consider it worth the effort.
Do you even know a container that supports 6-bit VGA palette? Or palette
that needs gamma correction? A list of 16 or so palettes might be
possible...
And you haven't suggested what to when the container does not offer any
suitable way to store that palette.


> >> is important too, by altering the packet you are causing many sides effects.
> > 
> > Please be more specific. It is the _current_ demuxer code that completely munges
> > up the data, throwing away information how the palette was encoded,
> > sometimes even applying (lossy!) gamma correction and in the process,
> > my patch simply changes that so that the demuxer passes all
> > video-related data _unchanged_ to the decoder.
> > The only issue is that the format might looks e.g. like this:
> > fixed size video-related data
> > palette size
> > palette
> > size
> > variable size video data
> 
> AVPacket->palette_data is still simpler IMHO.

Huh? How would it be simpler? Instead of just passing the whole bunch of
data on, the demuxer now has to extract the palette. Ok, otherwise the
decoder has to extract, still seems pretty much the same to me.
That is of course unless you absolutely insist on not providing any API
packets to merge data together into a single AVPacket.

> Also now that we pass AVPacket to decode_video, we may put AVPalette in
> AVFormatContext and use the same api, and instruct that palette changed
> to the decoder with a flag in AVPacket.

Oh dear. Are you even aware of the issues with AVPaletteControl? Because
your suggestion works just as badly with buffering an multithreading
that we could just keep it.

> Also, IMHO a clear distinction must be done between libavformat and
> libavcodec, nothing should mandate the use of libavcodec decoder,
> although for these game formats libavcodec may be the only software
> decoder available.

I can't see how that speaks in favour of your suggestion, my changes
make the decoder pass stuff on with as little modification as possible
whereas your suggestion to me just like the current code seems likely
to do palette decoding in the demuxer, thus linking demuxer and decoder
much more together. Though I can't tell since you have not answered what
palette_data is supposed to contain exactly.