[FFmpeg-devel] [PATCH] HAM6/HAM8 support for IFF demuxer/decoder

[Martin Storsjö]

On Sun, 9 May 2010, Sebastian Vater wrote:

> Martin Storsj? a ?crit :
> > On Sun, 9 May 2010, Sebastian Vater wrote:
> >
> >   
> >> Stefano Sabatini a ?crit :
> >>     
> >>> On date Thursday 2010-05-06 23:23:20 +0200, Sebastian Vater encoded:
> >>>   
> >>>       
> >>>> Sebastian Vater a ?crit :
> >>>>     
> >>>>  /**
> >>>>   * Convert CMAP buffer (stored in extradata) to lavc palette format
> >>>>   */
> >>>> -int ff_cmap_read_palette(AVCodecContext *avctx, uint32_t *pal)
> >>>> +static int cmap_read_palette(AVCodecContext *avctx, uint32_t *pal)
> >>>>     
> >>>>         
> >>> This looks fine, but this change deserves a separate patch.
> >>>   
> >>>       
> >> Why? It's important to change this with HAM support, the old code
> >> doesn't really have any problems with it.
> >>     
> >
> > No, it is not important for this change. In this patch, you remove usage 
> > of ff_cmap_read_palette from libavformat/iff.c. You don't _need_ to change 
> > this now. The code will work just as well without changing this.
> >   
> 
> It is, simply because it is strictly illegal now to call
> cmap_read_palette starting with this patch.

Illegal for which parts - you still call it from within libavcodec/iff.c. 
I assume you meant that it's illegal to call it from outside of 
lavc/iff.c, right?

> The thing is when I keep the function as is, I have to keep the
> prototype declaration also in the header file.
> 
> But that is dangerous, because people could think that it's still legal
> to use it with this patch, but it isn't anymore!
> Or may I move the iff.h declaration of ff_cmap_read_palette in
> libavcodec/iff.c?

Ok, given that you change how the function behaves, removing it all at 
once could be warranted. But if you're afraid someone would use it, you 
could still make it static and remove it from iff.h in a separate patch 
which is applied immediately afterwards, leaving the window of someone 
"thinking it's legal" to the time between two successive commits.

So I guess either way is acceptable in this case.

// Martin