[FFmpeg-devel] [PATCH] Enables parsing DVD color palette directly from IFO file

[Nicolas George]

Le decadi 20 brumaire, an CCXXIII, TOYAMA Shin-ichi a écrit :
> Attached patch enables parsing DVD color palette directly from IFO 
> file via -palette option using syntax `-palette ifo:filename`
> 
> This is a straightforward implementation of the procedure 
> described in the following post to ffmpeg-user mailing list.
> 
> http://ffmpeg.org/pipermail/ffmpeg-user/2013-September/017584.html

Thanks for the patch. See comments below.

> From c9e41ac2fbf6e518372be3057e8e794745190496 Mon Sep 17 00:00:00 2001
> From: Shin-ichi Toyama <shin1 at wmail.plala.or.jp>
> Date: Mon, 10 Nov 2014 22:13:35 +0900
> Subject: [PATCH] Enables parsing DVD color palette directly from IFO file via
>  -palette option using syntax `-palette ifo:filename'
> 
> ---

>  libavcodec/dvdsubdec.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++

The update for the documentation is missing.

>  1 file changed, 47 insertions(+)
> 
> diff --git a/libavcodec/dvdsubdec.c b/libavcodec/dvdsubdec.c
> index bb28d9e..b09eb61 100644
> --- a/libavcodec/dvdsubdec.c
> +++ b/libavcodec/dvdsubdec.c
> @@ -28,6 +28,7 @@
>  #include "libavutil/opt.h"
>  #include "libavutil/imgutils.h"
>  #include "libavutil/avstring.h"
> +#include "libavutil/bswap.h"
>  
>  typedef struct DVDSubContext
>  {
> @@ -574,13 +575,59 @@ static int dvdsub_decode(AVCodecContext *avctx,
>  static void parse_palette(DVDSubContext *ctx, char *p)
>  {
>      int i;
> +    char ifo[_MAX_PATH];
> +    FILE *in;
> +    uint32_t sp_pgci, pgci, off_pgc, pgc;
> +    uint8_t c, Y, Cr, Cb, R, G, B;
>  
>      ctx->has_palette = 1;
> +

> +  if (strncmp(p, "ifo:", 4) == 0) {

This can be discussed, but I would prefer having a separate -ifo_file
option.

> +    strncpy (ifo, p+4, _MAX_PATH);

The string copy is useless: just use p+4 as file name. Furthermore,
_MAX_PATH is evil.

> +    if((in=fopen(ifo, "r"))==NULL){

Please respect the surrounding coding style: four spaces (no tabs) indent,
spaces between structure keyword and parentheses, spaces around operators,
opening block braces on the same line with spaces before.

> +      fprintf(stderr, "[dvdsubdec.c] Warning: Failed to open IFO file\n");

You must use av_log(). And please also add a translation of errno for the
cause of error.

> +      ctx->has_palette = 0;
> +      return;
> +    }
> +    /* Obtain Start Point of PGCI */
> +    fseek(in, 0xCC, SEEK_SET);
> +    fread(&sp_pgci, 4, 1, in);
> +    sp_pgci=av_be2ne32(sp_pgci);

> +    

Trailing spaces can not be committed in the official repository. There are
other cases below.

> +    /* Obtain Offset to VTS_PGCI */
> +    pgci = sp_pgci * 2048;
> +    fseek(in, pgci + 0x0C, SEEK_SET);
> +    fread(&off_pgc, 4, 1, in);
> +    off_pgc=av_be2ne32(off_pgc);

Missing failure tests.

> +    
> +    /* Obtain Color pallet Start Point */

Typo in comment.

> +    pgc = pgci + off_pgc;
> +    fseek(in, pgc + 0xA4, SEEK_SET);
> +    
> +    for(i=0;i<16;i++)
> +    {

> +      fread(&c,  1, 1, in);
> +      fread(&Y,  1, 1, in);
> +      fread(&Cr, 1, 1, in);
> +      fread(&Cb, 1, 1, in);

You could replace that with a single read into an array; that would not
allow to call the variables R, Cr, Cb, but that does not matter much.

> +
> +      /* YCrCb - RGB conversion */
> +      Cr = Cr - 128;
> +      Cb = Cb - 128;

> +      R = Y + Cr + (Cr>>2) + (Cr>>3) + (Cr>>5);
> +      G = Y - ((Cb>>2) + (Cb>>4) + (Cb>>5)) - ((Cr>>1) + (Cr>>3) + (Cr>>4) + (Cr>>5));
> +      B = Y + Cb + (Cb>>1) + (Cb>>2) + (Cb>>6);

Are these he official formulas? The shifts look like premature optimization.

> +
> +      ctx->palette[i] = (R<<16) + (G<<8) + B;

Are you sure neither R, G, B overflow? If they can, they must be clipped.

> +    }
> +    fclose(in);
> +  } else {
>      for(i=0;i<16;i++) {
>          ctx->palette[i] = strtoul(p, &p, 16);
>          while(*p == ',' || av_isspace(*p))
>              p++;
>      }
> +  }
>  }
>  
>  static int dvdsub_parse_extradata(AVCodecContext *avctx)

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20141110/816bd966/attachment.asc>