[FFmpeg-devel] [PATCH] Implement hdcd filtering

Benjamin St benjaminst123 at gmail.com
Wed Mar 23 12:39:30 CET 2016

> const?

 missing new line.


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

Please use FFMIN()


> +} 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
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.


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 ;;


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>

More information about the ffmpeg-devel mailing list