[MPlayer-dev-eng] [PATCH] deinterlace property fix

[Vicente Sendra]

--- 21/10/12, Alexander Strasser wrote:

> De: Alexander Strasser <eclipse7 at gmx.net>
> Asunto: Re: [MPlayer-dev-eng] [PATCH] deinterlace property fix
> Para: mplayer-dev-eng at mplayerhq.hu
> Fecha: domingo, 21 de octubre, 2012 21:08
> Alexander Strasser wrote:
> > Vicente Sendra wrote:
> > > I've previously send a fix for command.c in:
> > > Better double frame rate output and framedrop
> > > 
> > > But there was a bug in my code.
> > > 
> > > This new one fixes old problem:
> > > 
> > > Better handling of deinterlace property, returns
> M_PROPERTY_ERROR if it can't be changed (or read), and
> displays osd value correctly (when change was not successful
> it showed the opposite value).
> > > 
> > > And my bug:
> > > 
> > > if VFCTRL_GET_DEINTERLACE was not successfull,
> deinterlace value was undefined.
> > > 
> > > I send it as a separate fix, as it's unrelated to
> new framedrop system.
> > 
> >   thanks for splitting and submitting
> separately!
> > 
> >   I tested your patch and changed some
> minor mostly naming things.
> > Your patch works out for me in that the behaviour of
> the OSD is
> > improved. I attached the modified patch including the
> planned
> > commit message.
> > 
> >   I am somewhat unsure about the
> semantics of the return code though.
> > Should we really return M_PROPERTY_ERROR? At least when
> it is not
> > supported in the filter chain it generates some
> confusing noise in
> > mplayer's console output.
> > 
> >   Sorry for the delay, but I want to
> think about this a little bit
> > more. If I come to no good conclusion I will commit
> your patch as
> > attached in the hope that if there are better ways to
> do it they will
> > be implemented on top of your change eventually.
> 
>   Partially committed.
> 
>   I only committed the part relevant for the OSD
> output. After
> looking at other properties implemented in command.c I came
> to
> the conclusion that returning M_PROPERTY_ERROR is probably
> not
> the correct thing to do here. If it is a problem for you
> please
> explain me your use case.
> 
>   Thank you for your contribution!

If you look at properties for brightness, contrast, hue, saturation, if you can not set or get them (not handled by any filter in the filter chain), it returns either value from set_video_colors if > 0 (just 1: M_PROPERTY_OK) or M_PROPERTY_UNAVAILABLE if no filter handles it.

Maybe M_PROPERTY_ERROR is not the correct one, but, i think the right thing should be something like this:

at getting:
M_PROPERTY_OK if some vf handles it.
M_PROPERTY_UNAVAILABLE if no filter handles it or assume it is disabled and return M_PROPERTY_OK (just as it is right now)

at setting
M_PROPERTY_OK if some vf handles it.
M_PROPERTY_UNAVAILABLE if no filter handles it
maybe M_PROPERTY_DISABLED if no filter handles it and we want to set it to true

For me it's not logical to return M_PROPERTY_OK if the property was not set, it's more logical to return M_PROPERTY_UNAVAILABLE, so i can know if setting was successful, without having to request property one more time just to check if it was really set.