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 <sh...@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
signature.asc
Description: Digital signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
