[FFmpeg-devel] [PATCH]Support 32bit palette in targa

[Michael Niedermayer]

On Tue, Oct 09, 2012 at 02:49:07AM +0200, Carl Eugen Hoyos wrote:
> On Tuesday 09 October 2012 01:14:06 am Michael Niedermayer wrote:
> > On Mon, Oct 08, 2012 at 01:11:47PM +0200, Carl Eugen Hoyos wrote:
> > > Hi!
> > >
> > > The Targa specification and gimp agree that the targa palette
> > > may contain transparency information.
> > > ImageMagick is broken: It allows 32bit palette entries but
> > > reads them identically as 24bit entries leading to a broken
> > > ("shifted") image.
> > >
> > > Please comment, Carl Eugen
> > >
> > >  targa.c    |    5 +++++
> > >  targaenc.c |    6 +++---
> > >  2 files changed, 8 insertions(+), 3 deletions(-)
> > > 61f853c3def447bafd8dc06deaaacc9c7c204d32  patchtgapal32.diff
> > > diff --git a/libavcodec/targa.c b/libavcodec/targa.c
> > > index 339d7c4..5e9d2e9 100644
> > > --- a/libavcodec/targa.c
> > > +++ b/libavcodec/targa.c
> > > @@ -181,6 +181,7 @@ static int decode_frame(AVCodecContext *avctx,
> > >              return -1;
> > >          }
> > >          switch (csize) {
> > > +        case 32: pal_sample_size = 4; break;
> > >          case 24: pal_sample_size = 3; break;
> > >          case 16:
> > >          case 15: pal_sample_size = 2; break;
> > > @@ -201,6 +202,10 @@ static int decode_frame(AVCodecContext *avctx,
> > >                  return AVERROR_INVALIDDATA;
> > >              }
> > >              switch (pal_sample_size) {
> > > +            case 4:
> > > +                for (t = 0; t < colors; t++)
> > > +                    *pal++ = bytestream2_get_le32u(&s->gb);
> > > +                break;
> > >              case 3:
> > >                  /* RGB24 */
> > >                  for (t = 0; t < colors; t++)
> >
> > this is probably ok
> 
> Hunk applied.
> 
> > > diff --git a/libavcodec/targaenc.c b/libavcodec/targaenc.c
> > > index 2f22e94..cc082aa 100644
> > > --- a/libavcodec/targaenc.c
> > > +++ b/libavcodec/targaenc.c
> > > @@ -107,11 +107,11 @@ static int targa_encode_frame(AVCodecContext
> > > *avctx, AVPacket *pkt, pkt->data[1]  = 1;          /* palette present */
> > >          pkt->data[2]  = TGA_PAL;    /* uncompressed palettised image */
> > >          pkt->data[6]  = 1;          /* palette contains 256 entries */
> > > -        pkt->data[7]  = 24;         /* palette contains 24 bit entries
> > > */ +        pkt->data[7]  = 32;         /* palette contains 32 bit
> > > entries */ pkt->data[16] = 8;          /* bpp */
> > >          for (i = 0; i < 256; i++)
> > > -            AV_WL24(pkt->data + 18 + 3 * i, *(uint32_t *)(p->data[1] + i
> > > * 4)); -        out += 256 * 3;             /* skip past the palette we
> > > just output */ +            AV_WL32(pkt->data + 18 + 4 * i, *(uint32_t
> > > *)(p->data[1] + i * 4)); +        out += 256 * 4;             /* skip
> > > past the palette we just output */
> >
> > if i understand correctly this would break imagemagik reading tgas
> > from ffmpeg if so
> 
> (Only pal8 tga's.)
> 
> > i think the palette should be checked and only stored as 32bit when
> > its needed for some entry
> 
> As in attached?

LGTM


> 
> > also please add a regression test once encoder and decoder support it
> 
> Wouldn't this need transparency in vsynth1 or vsynth2?

if that is used directly, yes

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Asymptotically faster algorithms should always be preferred if you have
asymptotical amounts of data
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20121009/c1b9a911/attachment.asc>