[FFmpeg-cvslog] r25066 - trunk/libavcodec/imgconvert.c

[Reimar Döffinger]

On Fri, Sep 10, 2010 at 07:43:43PM +0100, M?ns Rullg?rd wrote:
> Reimar D?ffinger <Reimar.Doeffinger at gmx.de> writes:
> 
> > On Fri, Sep 10, 2010 at 06:32:35PM +0100, M?ns Rullg?rd wrote:
> >> Reimar D?ffinger <Reimar.Doeffinger at gmx.de> writes:
> >> 
> >> > On Fri, Sep 10, 2010 at 05:28:27PM +0100, M?ns Rullg?rd wrote:
> >> >> Reimar D?ffinger <Reimar.Doeffinger at gmx.de> writes:
> >> >> 
> >> >> > On Fri, Sep 10, 2010 at 04:36:19PM +0100, M?ns Rullg?rd wrote:
> >> >> >> Why the FUCK do all 8-bit pixel formats claim they have a palette?
> >> >> >
> >> >> > Because anyone actually using (e.g. for displaying) the data
> >> >> > then doesn't need to add a special case for every single of them.
> >> >> 
> >> >> But there is no palette.  How does pretending there is one when it
> >> >> isn't even allocated (hence the valgrind error) simplify anything?
> >> >
> >> > It's a bug if there is none, I know for sure that at least almost
> >> > all 8-bit formats did indeed have a palette in data[1] some time
> >> > ago.
> >> 
> >> And just what is in that palette?
> >> 
> >> In the case at hand, there is obviously _something_ there, but there's
> >> not enough of it.  Since you seem to think this makes sense, I invite
> >> you to fix the bug.  I'm not touching it further.
> >
> > I don't think there's anything at all there, it's just that crappy
> > hack of rawvideo decoder messing up (ok, ok, maybe I am being a bit
> > too extreme here).
> > av_image_fill_pointers just sets the palette pointer to right after
> > the image data. Which makes little sense and should probably at most
> > be done for PAL8 - otherwise it is inconsistent with avpicture_get_size.
> > Anyway that means in this case it points right into nothing.
> > rawdec.c needs a special case for all paletted formats and needs to set
> > data[1] to data initialized by ff_set_systematic_pal (normally
> > get_buffer would handle that, but that is not used for raw video).
> > A proper solution should of course clean the code up instead of making
> > it even more of a mess, but something like below code should be what
> > is needed:
> > Index: rawdec.c
> > ===================================================================
> > --- rawdec.c    (revision 25096)
> > +++ rawdec.c    (working copy)
> > @@ -25,8 +25,10 @@
> >   */
> >
> >  #include "avcodec.h"
> > +#include "imgconvert.h"
> >  #include "raw.h"
> >  #include "libavutil/intreadwrite.h"
> > +#include "libavcore/imgutils.h"
> >
> >  typedef struct RawVideoContext {
> >      unsigned char * buffer;  /* block of memory for holding one frame */
> > @@ -81,7 +83,13 @@
> >          avctx->pix_fmt = find_pix_fmt(pix_fmt_bps_avi, avctx->bits_per_coded_sample);
> >
> >      context->length = avpicture_get_size(avctx->pix_fmt, avctx->width, avctx->height);
> > +    if (avctx->pix_fmt != PIX_FMT_PAL8 &&
> > +        (av_pix_fmt_descriptors[avctx->pix_fmt].flags & PIX_FMT_PAL))
> > +        context->length = 256 * 4;
> >      context->buffer = av_malloc(context->length);
> > +    if (avctx->pix_fmt != PIX_FMT_PAL8 &&
> > +        (av_pix_fmt_descriptors[avctx->pix_fmt].flags & PIX_FMT_PAL))
> > +        ff_set_systematic_pal(context->buffer, avctx->pix_fmt);
> >      context->pic.pict_type = FF_I_TYPE;
> >      context->pic.key_frame = 1;
> 
> This looks to me like it's replacing the image with _only_ a palette.
> That doesn't seem quite right.

buffer is never used currently in the avctx->pix_fmt != PIX_FMT_PAL8 case,
so no.