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

[Baptiste Coudurier]

On Wed, Apr 15, 2009 at 09:00:36PM +0200, Reimar D?ffinger wrote:
> 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.

In WC3 ? I thought you were talking about this.
For WC3 I believe extradata can be used, maybe not but only a try
can tell us.

> > >>>> 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.

I 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...

I don't know but the survey might help us knowing this if some people
do know.

> And you haven't suggested what to when the container does not offer any
> suitable way to store that palette.

Well, some containers aren't suitable, that's all, it's like vorbis in
ts. No big deal IMHO.

> > >> 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.

Maybe not, what are the issues ? Please explain.

I understand the problem with race condition is because the demuxer
actually updates AVCodecContext palette directly, but IMHO that can be
fixed by letting application dealing with palette change.

If formats didn't judge pertinent to store the palette within the frame,
why are we obsessed in doing this ?

> > 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.

Well, your patch assumes that libavcodec is used unfortunately, and
that libs are updated at the same time too.

If we don't decode palette in demuxer, we could pass raw data to
decoder in some way. 

I just don't like appending palette in packet data because this forces
parsing.

-- 
Baptiste COUDURIER                              GnuPG Key Id: 0x5C1ABAAA    
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
checking for life_signs in -lkenny... no
FFmpeg maintainer                                  http://www.ffmpeg.org