[FFmpeg-devel] [PATCH] Implement hdcd filtering

[Benjamin St]

>
> const?
>
Fixed

 missing new line.

Fixed


> Here and in every other function, { must be on own, separate line like
> in every other filter.
>
Fixed

Please use FFMIN()

Fixed

> +} HDCDContext;
> > +
> > +
> > +static const AVOption hdcd_options[] = {
> > +     {NULL}
>
> Please remove this if its not going to be used.
>
Isn't this needed by  AVFILTER_DEFINE_CLASS ?

This will crash if there is >2 channels
> Either limit filter to stereo and mono or allocate this differently.
>

Also, consider dropping the entire CHANNEL_NUM define, HDCDs will always
> carry a stereo signal, so there's never going to be a need to change
> CHANNEL_NUM.
>
Modified to only accept stereo.

 This is wrong. Either use av_calloc or modify query formats to accepts
> only mono/stereo channel layout.

Modified to only accept stereo.

 What is hdcd? Could you put it into this long description?

Should be better know

Alphabetical order.

Fixed

This is getting into #define INCREMENT(x) (x++) territory, could you remove
> it and just use sample *= gain everywhere?
>
Removed it

No need to specify exactly how many entries the array has when you define
> it, just leave the brackets empty []. It doesn't matter that much, but it
> makes it easier to extend the array later on if you need to.

Ok, removed it.

Duplicated ;;

Removed


Thanks for review, Benjamin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Implement-high-definition-audio-cd-filtering.patch
Type: text/x-patch
Size: 175746 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160323/ec2b14e0/attachment.bin>