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

[Jonathan Baldwin]

Sorry for the delay, I was kind of busy.

On Mon, Jun 6, 2011 at 5:02 AM, Stefano Sabatini
<stefano.sabatini-lala at poste.it> wrote:
>> +openal_indev_extralibs="${OPENAL_LIBS-'-lopenal'}"
>
> is this required?...
>
>> +enabled openal     && require openal 'AL/al.h' alGetError ${OPENAL_LIBS-'-lopenal'} &&
>                                                             ^^^^^^^^^^^^^^^^^^^^^^^^^
> ...isn't this enough?
>

Fixed. I didn't know 'require' actually added the libs.

>
> OK, maybe we could prefer plain filenames like out.ogg, /tmp/x.ogg
> looks very UNIX-ish, you may alienate some Windows user ;-).
>

Done.

>
> What about using FFMIN(FFMIN(a,b), FFMIN(c,d))?
>

Done.

>
> You may put each entry on a separate line, and vertically align fields.
>

Wasn't completely sure what you meant by 'entry', 'field', and
'vertically align' but I tried anyways.

>
> return a generic AVERROR(EIO) or AVERROR(EINVAL), AVERROR(INT_MAX)
> semantics is undefined.
>
> Also providing the error string was a nice feature.
>

Used EIO.

>
> typo
>

Oops ;-)

>
> alcCaptureOpenDevice() return NULL in case of failure, so you can use
> just a single "fail" label and do:
>
> fail:
>    if (ad->device)
>        alcCaptureCloseDevice(ad->device);
>    return error;

Done.

>
> Printing the error here as in the previous version is a good idea
> (improves feedback).
>

Prints the error again.
Also in the case of an EINVAL error in read_header, "Did you use a
valid OpenAL capture device name?" is printed along with a list of
OpenAL capture devices on the system, as that is the most likely cause
of that error. This is also now documented.

>
> More style nits:
>
> static int read_close(AVFormatContext* ctx)
> {
>    al_data *ad = ctx->priv_data;
>
>    if (ad->device) {
>        alcCaptureStop(ad->device);
>        alcCaptureCloseDevice(ad->device);
>    }
>    return 0;
> }
>
> that is: 4 chars of indent, no space between fun_name and params, and
> "{" on a starting line for functions. No need to address these nits if
> you don't care, I can do it myself with a few emacs keystrokes before
> committing.
>

I tried to fix this, but my IDE apparently doesn't support having no
space between function name and params, so I went through manually.

>
> Please mention these options in the manual. See how I did it in the
> sdl section of the output devices chapter.
>

Revamped the documentation thanks to your suggestion.
Now organized into subsections.
Added a table listing the three most common generic AL implementations
including links to their web sites.
Added an options subsection.
Cleaned up examples, now in their own section. Added an example where
an invalid device name is used, and explained that it will fail.

It seems quite extensive compared to the documentation for other
indevs, but perhaps that's a good thing.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ff_oal_patch_revised_2.diff
Type: text/x-patch
Size: 14937 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110610/391d8758/attachment.bin>