[FFmpeg-devel] [RFC] cryptokey options

Michael Niedermayer michaelni
Mon Dec 3 01:03:24 CET 2007


On Sun, Dec 02, 2007 at 08:13:56PM +0100, Reimar D?ffinger wrote:
> Hello,
> On Fri, Oct 19, 2007 at 03:24:38PM +0200, Reimar D?ffinger wrote:
> > On Fri, Oct 19, 2007 at 02:45:45PM +0200, Baptiste Coudurier wrote:
> > > Reimar D?ffinger wrote:
> > > > On Fri, Oct 19, 2007 at 03:00:20PM +0300, Kostya wrote:
> > > >> On Fri, Oct 19, 2007 at 12:29:44PM +0200, Reimar D?ffinger wrote:
> > > >>> I tried to implement this via a generic "binary" option type, but it
> > > >>> does not seem to work, avfc->key stays NULL in the demuxer.
> > > >>> Maybe that is just because I don't know how to use ffmpeg though...
> > > >>> Fixes welcome.
> > > >> Preserve original 'bin' variable pointer.
> > > >> You seem to memcpy data _after_ decoded string.
> > > > 
> > > > Ouch, obviously. But that does not explain why key kept being NULL in
> > > > the demuxer...
> > > > 
> > > 
> > > I think problem is in opt_input_file:
> > > 
> > >  for(i=0; i<opt_name_count; i++){
> > >         const AVOption *opt;
> > >         double d= av_get_double(avformat_opts, opt_names[i], &opt);
> > >         if(!isnan(d) && (opt->flags&AV_OPT_FLAG_DECODING_PARAM))
> > >             av_set_double(ic, opt_names[i], d);
> > >     }
> > > 
> > > Here av_get_double for cryptokey returns nan.
> > 
> > Grmpf. That code does not even handle the existing string and
> > AVRational stuff correctly (though the later at least works "mostly").
> 
> Kostya fixed most of the problems, so here is an updated patch.
> Does it look okay? (I do not really know much about how this option
> stuff is supposed to work in the details)

[...]
> Index: ffmpeg.c
> ===================================================================
> --- ffmpeg.c	(revision 11086)
> +++ ffmpeg.c	(working copy)
> @@ -2681,6 +2681,12 @@
>          double d= av_get_double(avformat_opts, opt_names[i], &opt);
>          if(!isnan(d) && (opt->flags&AV_OPT_FLAG_DECODING_PARAM))
>              av_set_double(ic, opt_names[i], d);
> +        else if((opt->flags&AV_OPT_FLAG_DECODING_PARAM) && opt->type == FF_OPT_TYPE_BINARY){
> +            char str[256];
> +            int strlen = 256;
> +            if(av_get_string(avformat_opts, opt_names[i], &opt, str, strlen) != NULL)
> +                av_set_string(ic, opt_names[i], str);
> +        }
>      }
>      /* open the input file with generic libav function */
>      err = av_open_input_file(&ic, filename, file_iformat, 0, ap);

this is ugly

also the av_?et_double() above is wrong as it doesnt copy strings, well
actually the whole copying code is ugly, AVOptions where not intended to
be used like they are in ffmpeg.c currently
the idea was to parse the comman line and store that in the struct
if needed parse again, not like the current parse once into a dummy
struct and copy it around ...


> Index: libavcodec/opt.c
> ===================================================================
> --- libavcodec/opt.c	(revision 11089)
> +++ libavcodec/opt.c	(working copy)
> @@ -69,6 +69,7 @@
>      case FF_OPT_TYPE_RATIONAL:
>          if((int)num == num) *(AVRational*)dst= (AVRational){num*intnum, den};
>          else                *(AVRational*)dst= av_d2q(num*intnum/den, 1<<24);
> +        break;
>      default:
>          return NULL;
>      }

unrelated?


[...]
> @@ -114,6 +122,25 @@
>      }
>      if(!o || !val || o->offset<=0)
>          return NULL;
> +    if(o->type == FF_OPT_TYPE_BINARY){
> +        uint8_t *bin, *ptr;
> +        int len = strlen(val);
> +        if (len & 1) return NULL;
> +        len /= 2;
> +        ptr = bin = av_malloc(len);

where is this freed() ? it seems nowhere ...
a series of av_set_string() would leak ...


[...]
> @@ -67,6 +68,7 @@
>  #define AV_OPT_FLAG_SUBTITLE_PARAM  32
>  //FIXME think about enc-audio, ... style flags
>      const char *unit;
> +    int offset2;
>  } AVOption;

actually, this is unneeded
offset + sizeof(void*) should do


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Those who are too smart to engage in politics are punished by being
governed by those who are dumber. -- Plato 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20071203/a281f7d0/attachment.pgp>



More information about the ffmpeg-devel mailing list