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

[Måns Rullgård]

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.

-- 
M?ns Rullg?rd
mans at mansr.com