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

[Stefano Sabatini]

On date Thursday 2010-11-04 17:07:16 +0100, Cyril Russo encoded:
> Hi,
> 
>   I'm a FFmpeg library user. Recently, you've deprecated
> sws_getContext telling to use sws_alloc_context & sws_init_context
> instead.
> This looks like a bad choice to me, as they are not equivalent.
> 
> First, sws_getContext had these avantages:
> 1) Library's user didn't need to know what the SwsContext content was.
> 2) All in one function, with no possible error in handling the return code.
> 
> And these cons: (so far as I can understand)
> 1) not the same naming convention as the other libraries in FFmpeg project
> 2) API is fixed.
> 
> In my opinion, new code have these defects:
> 1) sws_init_context meaning isn't clear. What is it used for ? Why
> is there no equivalent to this init function in other libraries ?
> 2) SwsContext isn't described in swscale.h file, but it's required
> to set its member if you want to actually use the context.
> 3) Since SwsContext structure can change, the user code (mine) could
> break if you rename/remove a member of the struct.
> 4) sws_getCachedContext is still here but not deprecated, and the
> documentation refer to sws_getContext.
> 5) I finally have to duplicate the getContext code in my code, and
> handle all the possible errors myself.
> 
> For me, I would prefer the "blackbox" idea, and having a function
> that does the dirty work of filling the opaque structure member
> correctly, and not having to deal with it.
> So, for symmetry with the other libavX lib, maybe having a new
> function called:
> sws_create_context(getContext_args_here) would bring back all the
> required features.
> Usage would then be:
> SwsContext * context = sws_alloc_context();

> 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.

> Instead of:
> SwsContext * c = sws_alloc_context();
> if (c == NULL) { perror("error allocating the context"); return -1; }
>     c->flags= flags;
>     c->srcW= srcW;
>     c->srcH= srcH;
>     c->dstW= dstW;
>     c->dstH= dstH;
>     c->srcRange = handle_jpeg(&srcFormat);
>     c->dstRange = handle_jpeg(&dstFormat);
>     c->srcFormat= srcFormat;
>     c->dstFormat= dstFormat;
> 
>     if (param) {
>         c->param[0] = param[0];
>         c->param[1] = param[1];
>     }

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.

>     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.

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.

Also check:
http://thread.gmane.org/gmane.comp.video.ffmpeg.devel/118412
http://thread.gmane.org/gmane.comp.video.ffmpeg.devel/118413
-- 
FFmpeg = Friendly Fundamentalist Multipurpose Proud Exuberant God