[FFmpeg-devel] [RFC] Reintroduce sws_getContext() as sws_create_context()

[Cyril Russo]

Le 04/11/2010 20:46, Stefano Sabatini a ?crit :
>
>> if (sws_create_context(context, 234, 243, ...etc...) != 0) {
>> perror("error creating the context"); return -1; }
> This is inflexible and inconsistent with the FFmpeg API, in general a
> context is a complex beast and you may need to add new options without
> to change the interface, as the number and type of the parameters may
> change.
>
> BTW this is also the reason for which the old interface was
> deprecated.
Ok, then what about following what FFmpeg's dev have done so far, that 
is introducing av_func2, av_func3 version and so on, in that case ?
Again, as you says, the context is a complex beast, so not having to 
deal with its members would be better, IMHO.

> This is internal code and that's not the supposed way to deal with the
> swscale context, which is *opaque* so can't directly messed up by the
> user.
>
I agree that the context should be opaque. But, only with sws_alloc and 
sws_init, how do you set the source & destination dimensions, without 
touching the struct members ?
I know about av_set_XX functions, but the documentation doesn't say 
anything about them.


>>      sws_setColorspaceDetails(c, ff_yuv2rgb_coeffs[SWS_CS_DEFAULT],
>> c->srcRange, ff_yuv2rgb_coeffs[SWS_CS_DEFAULT] /* FIXME*/,
>> c->dstRange, 0, 1<<16, 1<<16);
>>      if(sws_init_context(c, srcFilter, dstFilter)<  0) {
>>          sws_freeContext(c);
>>          perror("error initializing the context");
>>          return -1;
>>      }
>>
>> Let me know if you think it's a good idea, before I try to write a patch.
> libavcodec API:
>
> avcodec_alloc_context() ->  creates a non-opaque context, sets the
> fields to the default values.
>
> the options can be set explicitely setting the fields or using the
> libavutil/opt.h API.
>


> avcodec_open() ->  opens the context, fails in case of invalid parameters
>
> avcodec_close() ->  closes the context and frees all the structs
> initialized in avcodec_open()
>
> av_freep(&context) ->  frees the allocated struct
>
> ...
>
> libswscale (new) API:
>
> swsc_alloc_context() ->  creates an opaque context, sets the values to
> the default values (check libswscale/options.c).
>
> The context is opaque, the options can only be set using the
> sws_setColorspaceDetails() or the libavutil/opt.h API.
>
Yes the context is opaque so you can't change the struct's members.
How do you set, for example, the source width, or the destination width, 
or the destination height with sws_setColorspaceDetails, or even sws_scale ?

So you're back to use alternative means (like av_set_ et al), meaning 
you have to *guess* what are the members of the context, and it's not 
clean at all.

Back before deprecation, getContext was a good way to do without dealing 
with the structure members and there's no alternative anymore.

> swscale_init_context() sets the src and dst filters and intializes the
> context, fails if the options are not valid, at this point (I
> suppose) options should not be changed again when using the context.
>
> sws_freeContext() frees all the allocated opaque context structs and
> the context itself
>
> I can't see major flaws in this API.
See the example above. In general, it's a pain to have to query the 
members of a "structure" at runtime, making the code more like an 
interpreter, which I really dislike.
The user code is more complex, the library maintainer code is harder 
too, and when it's failing, you have no real idea why it fails, since 
it's buried deep in the "interpreter" code.

> Also check:
> http://thread.gmane.org/gmane.comp.video.ffmpeg.devel/118412
> http://thread.gmane.org/gmane.comp.video.ffmpeg.devel/118413
I'll.
Thanks.
Cyril