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

[Antoine Cellerier]

> 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