[MPlayer-dev-eng] vo equalizer cleanup

[Ivan Kalvachev]

On 5/2/11, Reimar Döffinger <Reimar.Doeffinger at gmx.de> wrote:
> On Mon, May 02, 2011 at 01:15:22PM +0300, Ivan Kalvachev wrote:
>> On 5/2/11, Reimar Döffinger <Reimar.Doeffinger at gmx.de> wrote:
>> > On 2 May 2011, at 01:35, Ivan Kalvachev <ikalvachev at gmail.com> wrote:
>> >> However instead of creating new structures for GET and SET I decided
>> >> to reused the single vf_equalizer structure from video filter system.
>> >> This allows the structure to be directly passed through vf_vo.
>> >> Because I use the very same structure, some files also need to include
>> >> the vf.h .
>> >> I could change that if you object on it, put please provide a clean
>> >> way to ensure that the vo structure would be the same as the vf one.
>> >> I picked the same structure because my plan is to convert vo to the vf
>> >> api.
>> >
>> > In that case, why not add a vfcontrol.h header, include that from
>> > video_out_internal.h and move things over there as needed?
>>
>> Instead of creating new headers, I could just include vf.h from
>> video_out_internal.h .
>
> That includes a whole lot of other stuff, which I think is a really
> bad idea. The vos have a lot of OS-specific code, thus there is a
> particularly high risk of namespace collission and we should absolutely
> avoid including half of MPlayer's header files within them.
> Once you accidentially happen expand this inclusion mess so far that
> the vos end up including loader/ headers you're almost certainly
> at a "s..tuff hits the fan" stage.

vf.h actually includes very little stuff not related directly to the
video system.
So I think it is safe for now.

>> However this would break the rule for header files inclusion, that we
>> ported from FFmpeg. Aka - each file should include the headers
>> directly needed by the stuff it contains. So if I use vf_equalizer I
>> should include the file where it is defined - vf.h .
>
> Actually it was mostly that headers should compile stand-alone so that
> changes in a .c file will not suddenly cause compilation errors in
> an included .h file.
> For anything else common sense should be used.
> Here video_out.h defines VOCTRL_SET_EQUALIZER, so it really should
> also provide the struct since without it that define can't be used.
> And it's perfectly fine to assume that including whatever provides
> VOCTRL_SET_EQUALIZER will also provide the struct necessary to use it.
> However with you planned changes we might want to remove both from
> video_out.h in the long term, but that's nothing that has to be handled
> now.

I do intent to unify the VOCTRL/VFCTRL (at least make them use same
number values). This would remove a dozen of lines in vf_vo.
Actually I hope to make vo very similar to what ve is now and
replacing all VOCTRL with VFCTRL is part of that.

>> So do you still want me to include vf.h from video_out_internal.h, and
>> remove the vf.h inclusions from my patch?
>
> I mostly do not want all of vf.h to be pulled into (almost) all vos.
> I think it would be reasonable to avoid extra includes, but I don't
> have much of an opinion really.

Should I understand that you won't object on committing my patch as it is now?
Or do you want me to create vfcontrol.h ? (that would probably be
patch on its own, moving VFCTRL definitions and associated structs)

How about other aspects of the patch. What do you prefer about the
vo_xv_set/get_eq()? Can I apply the patch about them right away?

Best Regards.