[vlc-devel] [Submit] video filter module - for thehomebrewAmbiLight (AtmoLight)

[Andre Weber]

Hello Dionoea,

next try?
http://eldo.gotdns.com/vlc_atmo.zip
(online from 5pm till 1am - durring the week, GMT+1)

changes are less
----------------
configure.ac ... moved my stuff below where other effects are allready listed
                      (somewhere in the near of goom) I hope its right?
Changed the help string - so that it is right now.

removed trailing spaces, and windows linefeeds (hope that I forgot nothing)


Regards,

André

------
atmo



----- Original Message ----- 
From: "Antoine Cellerier" <dionoea at videolan.org>
To: <vlc-devel at videolan.org>
Sent: Wednesday, January 23, 2008 10:33 PM
Subject: Re: [vlc-devel] [Submit] video filter module - for thehomebrewAmbiLight (AtmoLight)


> > configure.diff - with check for windows or linux build, and option to
> > disable atmo durring configure. (--disable-atmo) if you don't like it.
>
> The help string shout read "default enabled" instead of "default
> disabled". And the who if block should be moved a bit lower in the file
> where other such configure flag checks are handled (for consitency).
>
> > > vlc_codecs.diff:
> > > Why do you need to define this structure? Can't you use the VLC API to
> > > save a picture to a file? (check the image vout module, or the snapshot
> > > feature in the vout core)
> > > If you only need it for internal communication between the VLC filter
> > > and the atmo driver, you should include the define in your private
> > > headers.
> > because I need it only internaly I moved the changes from there to my own
> > file "AtmoDefs.h" ... but I still think it could also be part of
> > "vlc_codecs.diff:" just to have a complete definition of a bitmap file
> > there.
>
> Sounds fine as long as you keep it private. (Although it sill kind of
> duplicates existing VLC functionality ... but that doesn't really
> matter)
>
> > > vout_intf.c:
> > > Looks fine :)
> > the same again - no changes just to not forget it
>
> Ok.
>
> > > atmo_video_filter.diff:
> > >>> You have windows line endings in some of the files.
> > removed ... through dos2unix
> >
> > >>> You have trailing spaces.
> > remove by a replace run with regular expression ;-) the lazy way
> >
> > >>> It would be better if your source files were in a subdirectory.
> > done
> >
> > >>>    Files don't mention a license (well they mention a README.txt file
> > >>>    which isn't present in the diff)
> > readme and license there (GPL2)
> >
> > >>>  * atmo.cpp:
> > >>>    Lines should be 80 characters max.
> > ok ... the - pervert part
>
> Ok.
>
> > >>>    You might want to include a long description of your module using
> > >>>    set_help() in the module definition (see example in
> > >>>    modules/audio_filter/channel_mixer/headphone.c). Links to a webpage
> > >>>    describing the setup would be more than welcome.
> > ok I tried my best -- currently linking to german wiki pages of the project
> > I hope somebody will translate it ...*g*
>
> Sounds good for a start :) We'll have to post a news item with a demo
> video once it's included in VLC (and some english instructions I hope).
>
> > >>>    The switch line 900 can be simplified.
> > done... I hope the way you wanted it?
>
> Looks fine :) While looking at the new line 900, I noticed that you had:
> +        if(p_sys->i_AtmoOldEffect != emLivePicture)
> +            AtmoSwitchEffect(p_filter, p_sys->i_AtmoOldEffect);
> +
> +        if(p_sys->i_AtmoOldEffect == emLivePicture)
> +            AtmoSetLiveSource(p_filter, lvsGDI);
> Couldn't that be changed to if() .... else ... ?
>
> > >>>    ExtractMiniImage_YUV: is this just a chroma convert + scale function?
> > >>>    If it is you could use the ImageConvert VLC function which would call
> > >>>    ffmpeg to do the chroma + scale transform.
> > stays there - because I need no interpolation just a fast copy... and "downscale"
> > - directly to my memory internal buffer....
>
> Ok.
>
> > >>>    Filter: calls to var_GetInteger/Bool/... are slow (especially since
> > >>>    your filter has loads of variables). You should update the values
> > >>>    only if they change (like in a variable callback).
> > remove polling - and replaced by callback -- which isn't currently called,
> > because of the issue? with the settings dialog - which isn't modifiying the
> > object variables... the code is still there so that I can improve this later.
> > (if it is required)
>
> You could build a live dialog like the v4l2 module does. It's just
> adding 1 or 2 lines of code in the Qt interface and setting the right
> variable properties in your module as well as declaring a "controls"
> variables to list all available controls at run time. That would be
> worthwile ... I'll let you decide :)
>
> I'll wait for your last fixes before commiting this to svn.
>
> Regards,
>
> -- 
> Antoine Cellerier
> dionoea
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> http://mailman.videolan.org/listinfo/vlc-devel

_______________________________________________
vlc-devel mailing list
To unsubscribe or modify your subscription options:
http://mailman.videolan.org/listinfo/vlc-devel