[FFmpeg-devel] [PATCH] OpenAL 1.1 Capture Support

[Jonathan Baldwin]

> On Windows openal library mostly named as "OpenAL32.lib"
  The environment variable OPENAL_LIBS is now used for OpenAL
library-related flags if it exists, otherwise -lopenal is used.
Windows builders can set the environment variable appropriately.

>
> I think this could be simplified by using:
> enabled libopenal && require AL/al.h alGetError -lopenal &&
>                      { check_cpp_condition AL/al.h "defined(AL_VERSION_1_1)" ||
>                        die "ERROR: libopenal version must be 1.1"; }
> ...
> openal_indev_deps="libopenal"
>

Added "openal" to CONFIG_LIST, added --enable-openal switch to
show_help, added echo "openal enabled            ${openal-no}" to
where all the other ones are, changed openal_indev_deps so openal is
the sole dep, removed "enabled openal_indev && ..." cruft, added
something similar to above but with openal instead of libopenal.

>
> "$" alone should be more clear.
>

Done.

>
> nit: @file{openal-info}
>

Done.

>
> it is your OpenAL implementation => it may be fault of your OpenAL implemention
>
> since we're no perfect.
>

Added the word "probably" ;-)

>
> Nit++: create a dedication /** @file ... */ doxy for this
>

Done at top of file.

>
> OK, if you want this to be released as public domain (rather than the
> standard LGPL adopted by most FFmpeg codebase).
>

I prefer that the freedom of my code is enforced by moral rather than
legal matters.

>
> Style nits:
> * "typedef struct {" on the same line
> * if (foo) {         on the same line, (foo) is preferred over ( foo )
> * switch (foo) {     on the same line
>

I prefer that style too, but my IDE disagreed with me. I changed its
settings so now everything should be fine. Fixed.

>
> AV_NE(CODEC_ID_PCM_S16BE, CODEC_ID_PCM_S16LE),
>

I knew AV_NE existed but forgot its name. I tried grepping for ENDIAN
but couldn't find it. Fixed.

>
> This information can be stored in a table
>
> struct al_format_entry {
>   ALCenum al_fmt, enum AVSampleFormat sample_fmt; int nb_channels; int sample_size;
> } al_format_entries[] = {
>  ...
> }
>
> sample_size can be computed from nb_channels and sample_fmt by using
> av_get_bits_per_sample_fmt().
>

Added a lookup table encapsulated in an inline get_ function, removed
old al_*( alf ) crufty functions.

>
> Maybe you can directly return the error code, and somehow simplify the
> interface.
>

Done. I hope.

>
> NULL is preferred over 0 for struct pointers.
>

Done.

>
> maybe this can be safely skipped
>

That block of code is now removed.

>
> Nit: third person, also no need to document standard callback imo.
>

Documentation for standard read_* callbacks removed.

>
> Thanks for the patch.
> --

My pleasure. An updated patch is attached.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ff_oal_patch_revised.diff
Type: text/x-patch
Size: 12222 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110605/f18d0dfb/attachment.bin>