[FFmpeg-devel] [PATCH] Move param initialization and colorspace details from sws_getContext() to sws_init_context().

[Stefano Sabatini]

On date Thursday 2010-09-30 01:34:14 +0200, Michael Niedermayer encoded:
> On Thu, Sep 30, 2010 at 12:56:20AM +0200, Stefano Sabatini wrote:
> > On date Wednesday 2010-09-29 19:25:28 +0200, Michael Niedermayer encoded:
> > > On Wed, Sep 29, 2010 at 06:00:27PM +0200, Stefano Sabatini wrote:
> > > > Allow to automatically set the default parameters without explicitely
> > > > set them, also simplify conversion to the new sws_init_context() API.
> > > > ---
> > > >  utils.c |   18 +++++++-----------
> > > >  1 files changed, 7 insertions(+), 11 deletions(-)
> > > > 
> > > > diff --git a/utils.c b/utils.c
> > > > index 938af9e..bee5a3a 100644
> > > > --- a/utils.c
> > > > +++ b/utils.c
> > > > @@ -761,10 +761,17 @@ int sws_init_context(SwsContext *c, SwsFilter *srcFilter, SwsFilter *dstFilter)
> > > >      int dstW= c->dstW;
> > > >      int dstH= c->dstH;
> > > >      int flags;
> > > > +    c->srcRange = handle_jpeg(&c->srcFormat);
> > > > +    c->dstRange = handle_jpeg(&c->dstFormat);
> > > 
> > > that breaks user setted *Range
> > 
> > I'm aware but I cannot see a simpler way. A possibility would be to
> > introduce an hack external to this function, but then doesn't look
> > that good as well.
> > 
> > The problem is: how should the new API deal with JPEG colorspaces?
> > Currently when we want to use the JPEG colorspace we pass one of the
> > YUVJ* pixel formats. Avoiding this hack would mean to make the various
> > FFmpeg components (well, mainly codecs) explicitely define which
> > colorspace/range to use.
> 
> YUVJ* is deprecated
> so designing new code toward it doesnt feel very good at all
> 
> also i dont see a problem
> if YUVJ set Range appropriately
> if not dont touch Range and leave it at the default / what the usert did set

Currently this has to be done directly in the application (in this
case ffmpeg.c, right?), so I was thinking about creating a copy of
handle_jpeg(), which is ugly, while it should be duty of libavcodec to
do that, but I have yet to think which would be the better way.

And patch updated with a more minimal change.

> [...]
> > > > +
> > > > +    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 ARCH_X86
> > > >      if (flags & SWS_CPU_CAPS_MMX)
> > > >          __asm__ volatile("emms\n\t"::: "memory");
> > > 
> > > doing that before emms sounds like a really stupid idea, are you sure theres
> > > no float code in there and never will be ...
> > 
> > Uh? For the very little that I know emms re-allows FPU instruction use
> > after some MMX instructions were used, in this case I don't see why
> > this would be a proble, since sws_setColorspaceDetails() is called
> > *before* this. Also I'm simply keeping the same sequential order as in
> > the original sws_getContext()...
> 
> i think we can remove this emms

I'll leave that to someone more asm-savvy than me.

Regards.
-- 
FFmpeg = Faithless and Fast Most Proud Exuberant Governor