Re: [FFmpeg-devel] [PATCH] Implement hdcd filtering

2016-04-15 Thread Carl Eugen Hoyos 
Derek Buitenhuis  gmail.com> writes:

> Why is this in libavfilter and not libavcodec, when it is 
> described as a decoder?

Please fix the documentation, neither the author nor myself 
are native speakers.

Carl Eugen

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Implement hdcd filtering

2016-04-15 Thread Derek Buitenhuis 
On 3/22/2016 11:22 AM, Benjamin St wrote:
> This patch applies filtering/decoding for hdcds(see ticket #4441) . The
> filter is heavily based on
> https://github.com/kode54/foo_hdcd/. (Is this ok? Copyright?)
> 
> Discuss, Review

Hi,

Sorry to be late to the party (I just saw this pushed.)

Why is this in libavfilter and not libavcodec, when it is described
as a decoder?

- Derek
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Implement hdcd filtering

2016-04-15 Thread Carl Eugen Hoyos 
Benjamin St  gmail.com> writes:

> OK, added av_assert0() checks.

Tested again and applied.

Thank you, Carl Eugen

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Implement hdcd filtering

2016-04-13 Thread Michael Niedermayer 
On Tue, Apr 12, 2016 at 04:28:20PM +0200, Benjamin St wrote:
> >
> > Please compress the input sample with a lossless audio
> > encoder.
> >
> Ok, input is know compressed with flac.
> 
> 
>  Note that you also have to send a patch that adds the
> > new license to allow fate to pass.
> 
> Fate is passing know.
> Seems that
> 
> >  Redistribution and use in source and binary forms, with or without
> > modification
> >
> is already ok. It just need to be in one line.

[...]

> +static int hdcd_envelope(int *samples, int count, int stride, int gain, int 
> target_gain, int extend)
> +{
> +int i;
> +

please add somewhere a av_assert0() that ensures that the length
that is written to is within the number of samples
the code that sets that value is complex and spread out enugh that
i think this should be explicitly checked

no more comments from me, further review & apply left to others

thanks

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you drop bombs on a foreign country and kill a hundred thousand
innocent people, expect your government to call the consequence
"unprovoked inhuman terrorist attacks" and use it to justify dropping
more bombs and killing more people. The technology changed, the idea is old.


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Implement hdcd filtering

2016-04-13 Thread Michael Niedermayer 
On Tue, Apr 12, 2016 at 04:28:20PM +0200, Benjamin St wrote:
> >
> > Please compress the input sample with a lossless audio
> > encoder.
> >
> Ok, input is know compressed with flac.

flac file uploded

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The greatest way to live with honor in this world is to be what we pretend
to be. -- Socrates


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Implement hdcd filtering

2016-04-11 Thread Carl Eugen Hoyos 
Benjamin St  gmail.com> writes:

> Attachment (hdcd.wav): audio/x-wav, 1584 KiB

Please compress the input sample with a lossless audio 
encoder.

Note that you also have to send a patch that adds the 
new license to allow fate to pass.

Carl Eugen

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Implement hdcd filtering

2016-04-09 Thread Michael Niedermayer 
On Sat, Apr 09, 2016 at 11:35:03PM +0200, Benjamin St wrote:
> >
> > Changelog entry and docs update missing
> >
> Fixed
> 
>  has this been tested against some reference / known to be correct
> > output for some stream ?
> 
> It has been tested again examples uploaded at ticket #4441
> 

>  a fate test would still be interresting i think
> 
> If a fate test is really needed (floats are no longer used), I could use
> the files provided at the ticket as a reference.

its not "really needed" but it would good if code is tested so that
regressions are found early.

the filter also seems not to check its input parameters
./ffmpeg -v 99 -i HDCD.flac -af aresample=och=6,hdcd test.wav

segfaults

so does
./ffmpeg -v 99 -i HDCD.flac -af aresample=32000,hdcd test.wav

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The real ebay dictionary, page 3
"Rare item" - "Common item with rare defect or maybe just a lie"
"Professional" - "'Toy' made in china, not functional except as doorstop"
"Experts will know" - "The seller hopes you are not an expert"


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Implement hdcd filtering

2016-04-09 Thread Carl Eugen Hoyos 
Michael Niedermayer  niedermayer.cc> writes:

> has this been tested against some reference / known to 
> be correct output for some stream ?

Ticket #4441 contains a file HDCD16.flac with an original 
stream from a DVD and HDCD32.flac which contains the 
output as produced by the Windows tool to convert HDCD.

Carl Eugen

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Implement hdcd filtering

2016-04-09 Thread Carl Eugen Hoyos 
Michael Niedermayer  niedermayer.cc> writes:

> On Thu, Apr 07, 2016 at 03:21:46PM +0200, Benjamin St wrote:
> > > if the license is compatible with the LGPL then you could 
> > > be added to the exceptions
> > >
> > Is it compatible? I would say yes... (but I'm not sure)
> 
> ill leave that to carl, he knows more about licenses and such
> either way, "make fate" must pass after applying the patch

The license is nearly identical with the three-clause BSD 
license and is compatible with both the LGPL and the GPL.
Benjamin, please send a patch that adds it to the relevant 
fate test.

Thank you, Carl Eugen

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Implement hdcd filtering

2016-04-07 Thread Michael Niedermayer 
On Tue, Mar 29, 2016 at 11:30:28AM +0200, Benjamin St wrote:
> > Not all of the comments on top of the filter file look
> > very useful to me, what do you think?
> > In any case, I'd say a link to these should be useful:
> > http://www.audiomisc.co.uk/HFN/HDCD/Enigma.html
> > http://www.audiomisc.co.uk/HFN/HDCD/Examined.html
> > (Or only the second one.)
> >
> I removed the rather useless comment and added both links.
> 
> Please also add a fate test, since this is using floats,
> > I would suggest to only compare pcm_s16 (instead of 24).
> 
> Floats are no longer needed. This version uses only integer.

>  Makefile |1 
>  af_hdcd.c| 1138 
> +++
>  allfilters.c |1 
>  3 files changed, 1140 insertions(+)
> 4e25a0315a659eb9e71adfacb9f9b2e78c5e1f33  
> 0001-Implement-high-definition-audio-cd-filtering.patch
> From f3053beac96884123e1c9d50b46876e6f69ae0d1 Mon Sep 17 00:00:00 2001
> From: Benjamin Steffes 
> Date: Mon, 21 Mar 2016 23:52:48 +0100
> Subject: [PATCH] Implement high definition audio cd filtering.
> 
> Signed-off-by: Benjamin Steffes 
> ---
>  libavfilter/Makefile |1 +
>  libavfilter/af_hdcd.c| 1138 
> ++
>  libavfilter/allfilters.c |1 +
>  3 files changed, 1140 insertions(+)
>  create mode 100644 libavfilter/af_hdcd.c

Changelog entry and docs update missing

a fate test would still be interresting i think

has this been tested against some reference / known to be correct
output for some stream ?

[...]

> +static int integrate(hdcd_state_t *state, int *flag, int const *samples, int 
> count, int stride)
> +{
> +uint32_t bits = 0;
> +int result = FFMIN(state->readahead, count);
> +int i;
> +*flag = 0;
> +
> +for (i = result - 1; i >= 0; i--) {
> +bits |= (*samples & 1) << i; /* might be better as a conditional? */
> +samples += stride;
> +}
> +
> +state->window = (state->window << result) | bits;
> +state->readahead -= result;
> +if (state->readahead > 0)
> +return result;
> +

> +bits = (state->window ^ state->window >> 5 ^ state->window >> 23) & 
> 0xu;

the & 0xu; seems redundant


[...]
> +static int filter_frame(AVFilterLink *inlink, AVFrame *in)
> +{
> +AVFilterContext *ctx = inlink->dst;
> +HDCDContext *s = ctx->priv;
> +AVFilterLink *outlink = ctx->outputs[0];
> +AVFrame *out;

> +int16_t *in_data;

const

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No snowflake in an avalanche ever feels responsible. -- Voltaire


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Implement hdcd filtering

2016-04-07 Thread Michael Niedermayer 
On Thu, Apr 07, 2016 at 03:21:46PM +0200, Benjamin St wrote:
> > if the license is compatible with the LGPL then you could be added to
> > the exceptions
> >
> Is it compatible? I would say yes... (but I'm not sure)

ill leave that to carl, he knows more about licenses and such
either way, "make fate" must pass after applying the patch


> Any further comments?

let me take another look

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I know you won't believe me, but the highest form of Human Excellence is
to question oneself and others. -- Socrates


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Implement hdcd filtering

2016-04-07 Thread Benjamin St 
> if the license is compatible with the LGPL then you could be added to
> the exceptions
>
Is it compatible? I would say yes... (but I'm not sure)
Any further comments?

Thanks, Benjamin
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Implement hdcd filtering

2016-03-30 Thread Michael Niedermayer 
On Wed, Mar 30, 2016 at 11:42:47AM +0200, Benjamin St wrote:
> >
> > fails make fate
> 
> It's due to the copyright header. I still don't know how to handle this
> correct as this filter is based on https://github.com/kode54/foo_hdcd/,
> which contains:
> 
> > 1. Redistributions of source code must retain the above copyright
> > notice, this list of conditions and the following disclaimer.

if the license is compatible with the LGPL then you could be added to
the exceptions

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Freedom in capitalist society always remains about the same as it was in
ancient Greek republics: Freedom for slave owners. -- Vladimir Lenin


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Implement hdcd filtering

2016-03-30 Thread Benjamin St 
>
> fails make fate

It's due to the copyright header. I still don't know how to handle this
correct as this filter is based on https://github.com/kode54/foo_hdcd/,
which contains:

> 1. Redistributions of source code must retain the above copyright
> notice, this list of conditions and the following disclaimer.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Implement hdcd filtering

2016-03-29 Thread Michael Niedermayer 
On Tue, Mar 29, 2016 at 11:30:28AM +0200, Benjamin St wrote:
> > Not all of the comments on top of the filter file look
> > very useful to me, what do you think?
> > In any case, I'd say a link to these should be useful:
> > http://www.audiomisc.co.uk/HFN/HDCD/Enigma.html
> > http://www.audiomisc.co.uk/HFN/HDCD/Examined.html
> > (Or only the second one.)
> >
> I removed the rather useless comment and added both links.
> 
> Please also add a fate test, since this is using floats,
> > I would suggest to only compare pcm_s16 (instead of 24).
> 
> Floats are no longer needed. This version uses only integer.

>  Makefile |1 
>  af_hdcd.c| 1138 
> +++
>  allfilters.c |1 
>  3 files changed, 1140 insertions(+)
> 4e25a0315a659eb9e71adfacb9f9b2e78c5e1f33  
> 0001-Implement-high-definition-audio-cd-filtering.patch
> From f3053beac96884123e1c9d50b46876e6f69ae0d1 Mon Sep 17 00:00:00 2001
> From: Benjamin Steffes 
> Date: Mon, 21 Mar 2016 23:52:48 +0100
> Subject: [PATCH] Implement high definition audio cd filtering.
> 
> Signed-off-by: Benjamin Steffes 
> ---
>  libavfilter/Makefile |1 +
>  libavfilter/af_hdcd.c| 1138 
> ++
>  libavfilter/allfilters.c |1 +
>  3 files changed, 1140 insertions(+)
>  create mode 100644 libavfilter/af_hdcd.c

fails make fate

--- ./tests/ref/fate/source 2016-03-28 20:55:31.220829289 +0200
+++ tests/data/fate/source  2016-03-29 23:51:43.338872226 +0200
@@ -8,6 +8,7 @@
 libavcodec/mathops.c
 libavcodec/reverse.c
 libavdevice/file_open.c
+libavfilter/af_hdcd.c
 libavfilter/log2_tab.c
 libavformat/file_open.c
 libavformat/golomb_tab.c
Test source failed. Look at tests/data/fate/source.err for details.
make: *** [fate-source] Error 1
make: *** Waiting for unfinished jobs

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

it is not once nor twice but times without number that the same ideas make
their appearance in the world. -- Aristotle


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Implement hdcd filtering

2016-03-26 Thread Carl Eugen Hoyos 
Benjamin St  gmail.com> writes:

> I tried to test with the files sample.cdda.flac and
> > sample.hdcd.flac attached in the ticket.
> > The output is different and one possible reason
> > is the usage of floating point arithmetic in the
> > filter.
> 
> Did you compare flac files or raw audiodata (e.g. wav)?

I did compare wav but I probably made a mistake:
I can confirm that the output is bitexact with the 
Windows app and your patch.

> For me flac it is not identical but wav is.

> (probably due to different flac encoders?)

I also used the FFmpeg flac encoder when producing the 
sample file...

Not all of the comments on top of the filter file look 
very useful to me, what do you think?
In any case, I'd say a link to these should be useful:
http://www.audiomisc.co.uk/HFN/HDCD/Enigma.html
http://www.audiomisc.co.uk/HFN/HDCD/Examined.html
(Or only the second one.)

Please also add a fate test, since this is using floats, 
I would suggest to only compare pcm_s16 (instead of 24).

Carl Eugen

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Implement hdcd filtering

2016-03-24 Thread Benjamin St 
I tried to test with the files sample.cdda.flac and
> sample.hdcd.flac attached in the ticket.
> The output is different and one possible reason
> is the usage of floating point arithmetic in the
> filter.
>

Did you compare flac files or raw audiodata (e.g. wav)? For me flac it is
not identical but wav is.(probably due to different flac encoders?)
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Implement hdcd filtering

2016-03-23 Thread Carl Eugen Hoyos 
Benjamin St  gmail.com> writes:

> This patch applies filtering/decoding for hdcds(see 
> ticket #4441)

I tried to test with the files sample.cdda.flac and 
sample.hdcd.flac attached in the ticket.
The output is different and one possible reason 
is the usage of floating point arithmetic in the 
filter. Since the input and output formats are 
integer formats, can't the usage of floats be 
avoided?

Carl Eugen

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Implement hdcd filtering

2016-03-23 Thread Carl Eugen Hoyos 
Benjamin St  gmail.com> writes:

> This patch applies filtering/decoding for hdcds(see ticket #4441) . 
> The filter is heavily based on
> https://github.com/kode54/foo_hdcd/. (Is this ok? 

It seems to me as if the most simple solution is not 
to change the license but add the license to our 
used licenses.

> Copyright?)

If you added something important, you should of course 
add your copyright.

Carl Eugen

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Implement hdcd filtering

2016-03-23 Thread Michael Niedermayer 
On Wed, Mar 23, 2016 at 12:39:30PM +0100, Benjamin St wrote:
> >
> > 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

>  Makefile |1 
>  af_hdcd.c| 1264 
> +++
>  allfilters.c |1 
>  3 files changed, 1266 insertions(+)
> 76d7406d2f7732c615981ab9c0689294a37eec72  
> 0001-Implement-high-definition-audio-cd-filtering.patch
> From 1bc760b84592eff4bb923a23a1415f7d42a4aaf2 Mon Sep 17 00:00:00 2001
> From: Benjamin Steffes 
> Date: Mon, 21 Mar 2016 23:52:48 +0100
> Subject: [PATCH] Implement high definition audio cd filtering.
> 
> Signed-off-by: Benjamin Steffes 
> ---
>  libavfilter/Makefile |1 +
>  libavfilter/af_hdcd.c| 1264 
> ++
>  libavfilter/allfilters.c |1 +
>  3 files changed, 1266 insertions(+)
>  create mode 100644 libavfilter/af_hdcd.c
> 
> diff --git a/libavfilter/Makefile b/libavfilter/Makefile
> index be4b3c1..2b9bc84 100644
> --- a/libavfilter/Makefile
> +++ b/libavfilter/Makefile
> @@ -103,6 +103,7 @@ OBJS-$(CONFIG_TREMOLO_FILTER)+= 
> af_tremolo.o
>  OBJS-$(CONFIG_VIBRATO_FILTER)+= af_vibrato.o 
> generate_wave_table.o
>  OBJS-$(CONFIG_VOLUME_FILTER) += af_volume.o
>  OBJS-$(CONFIG_VOLUMEDETECT_FILTER)   += af_volumedetect.o
> +OBJS-$(CONFIG_HDCD_FILTER)   += af_hdcd.o
>  
>  OBJS-$(CONFIG_AEVALSRC_FILTER)   += aeval.o
>  OBJS-$(CONFIG_ANOISESRC_FILTER)  += asrc_anoisesrc.o
> diff --git a/libavfilter/af_hdcd.c b/libavfilter/af_hdcd.c
> new file mode 100644
> index 000..63ad309
> --- /dev/null
> +++ b/libavfilter/af_hdcd.c
> @@ -0,0 +1,1264 @@
> +/*
> +   Copyright (C) 2010, Chris Moeller,
> +   All rights reserved.
> +   Optimizations by Gumboot
> +   Redistribution and use in source and binary forms, with or without
> +   modification, are permitted provided that the following conditions
> +   are met:
> + 1. Redistributions of source code must retain the above copyright
> +notice, this list of conditions and the following disclaimer.
> + 2. Redistributions in binary form must reproduce the above copyright
> +notice, this list of conditions and the following disclaimer in the
> +documentation and/or other materials provided with the distribution.
> + 3. The names of its contributors may not be used to endorse or promote
> +products derived from this software without specific prior written
> +permission.
> +   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> +   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> +   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> +   A PARTICULAR PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL THE COPYRIGHT 
> OWNER OR
> +   CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
> +   EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
> +   PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
> +   PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
> +   LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
> +   NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
> +   SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> +*/
> +
> +/*
> +  Original code reverse engineered from HDCD decoder library by Christopher 
> Key,
> +  which was likely reverse engineered

Re: [FFmpeg-devel] [PATCH] Implement hdcd filtering

2016-03-23 Thread Paul B Mahol 
On 3/23/16, Benjamin St  wrote:
> Isn't this needed by  AVFILTER_DEFINE_CLASS ?

You dont need it if filter doesnt have options to set.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Implement hdcd filtering

2016-03-22 Thread Rostislav Pehlivanov 
On 22 March 2016 at 11:22, Benjamin St  wrote:

> This patch applies filtering/decoding for hdcds(see ticket #4441) . The
> filter is heavily based on
> https://github.com/kode54/foo_hdcd/. (Is this ok? Copyright?)
>
> Discuss, Review
>
> Thank you, Benjamin
>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>
Some style issues:

>+static int hdcd_scan(hdcd_state_t *state, int const *samples, int max,
int stride) {
In FFmpeg we always put the opening function bracket on a new line.

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

>+static const uint32_t peaktab[9856] =
>+static uint8_t readaheadtab[1 << 8] = {
>+static float gaintab[(15 << 7) + 1] = {
>++static float gaintabup[(15 << 7) + 1] = {
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.

>out = ff_get_audio_buffer(outlink, in->nb_samples);;
Duplicated ;;

>+static void hdcd_process(hdcd_state_t *state, int *samples, int count,
>+int stride) {
Not sure if the attachmed is messed up, but align the argument on the
newline to be after the bracket like:
>+static void hdcd_process(hdcd_state_t *state, int *samples, int count,
>+int stride) {

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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Implement hdcd filtering

2016-03-22 Thread Paul B Mahol 
On 3/22/16, Benjamin St  wrote:
> This patch applies filtering/decoding for hdcds(see ticket #4441) . The
> filter is heavily based on
> https://github.com/kode54/foo_hdcd/. (Is this ok? Copyright?)
>
> Discuss, Review
>
> Thank you, Benjamin
>

[...]

> +};
> +
> +static uint8_t readaheadtab[1 << 8] = {

const?

> +0x03, 0x02, 0x01, 0x01, 0x1f, 0x1e, 0x1f, 0x11, 0x1f, 0x1e, 0x1f, 
> 0x1d, 0x1f, 0x1e, 0x1f,
> +0x10, 0x1f, 0x1e, 0x1f, 0x1d, 0x1f, 0x1e, 0x1f, 0x1c, 0x1f, 0x1e, 
> 0x1f, 0x1d, 0x1f, 0x1e,
> +0x1f, 0x0f, 0x1f, 0x1e, 0x1f, 0x1d, 0x1f, 0x1e, 0x1f, 0x1c, 0x1f, 
> 0x1e, 0x1f, 0x1d, 0x1f,
> +0x1e, 0x1f, 0x1b, 0x1f, 0x1e, 0x1f, 0x1d, 0x1f, 0x1e, 0x1f, 0x1c, 
> 0x1f, 0x1e, 0x1f, 0x1d,
> +0x1f, 0x1e, 0x0e, 0x19, 0x07, 0x1e, 0x1f, 0x1d, 0x1f, 0x1e, 0x1f, 
> 0x1c, 0x1f, 0x1e, 0x1f,
> +0x1d, 0x1f, 0x1e, 0x1f, 0x1b, 0x1f, 0x1e, 0x1f, 0x1d, 0x1f, 0x1e, 
> 0x1f, 0x1c, 0x1f, 0x1e,

[...]

> +
> +static float gaintab[(15 << 7) + 1] = {

const?

[...]

> +0.427424, 0.427232, 0.427040, 0.426848, 0.426656, 0.426464, 0.426273, 
> 0.426081, 0.425889, 0.425698, 0.425507, 0.425315, 0.425124, 0.424933, 
> 0.424742,
> +0.424551, 0.424360, 0.424169, 0.423978, 0.423788, 0.423597, 0.423407, 
> 0.423216, 0.423026, 0.422836, 0.422646, 0.422456, 0.422266, 0.422076, 
> 0.421886,
> +0.421697
> +};

missing new line.

> +static float gaintabup[(15 << 7) + 1] = {

const?

> +1.00, 0.999550, 0.999101, 0.998652, 0.998203, 0.997754, 0.997305, 
> 0.996857, 0.996409, 0.995961, 0.995513, 0.995065, 0.994618, 0.994171, 
> 0.993724,
> +0.993277, 0.992830, 0.992384, 0.991938, 0.991492, 0.991046, 0.990600, 
> 0.990155, 0.989710, 0.989265, 0.988820, 0.988375, 0.987931, 0.987487, 
> 0.987043,

[...]

> +0.433230, 0.433035, 0.432841, 0.432646, 0.432452, 0.432257, 0.432063, 
> 0.431869, 0.431674, 0.431480, 0.431286, 0.431092, 0.430899, 0.430705, 
> 0.430511,
> +0.430318, 0.430124, 0.429931, 0.429737, 0.429544, 0.429351, 0.429158, 
> 0.428965, 0.428772, 0.428579, 0.428387, 0.428194, 0.428002, 0.427809, 
> 0.427617,
> +0.427424, 0.427232, 0.427040, 0.426848, 0.426656, 0.426464, 0.426273, 
> 0.426081, 0.425889, 0.425698, 0.425507, 0.425315, 0.425124, 0.424933, 
> 0.424742,
> +0.424551, 0.424360, 0.424169, 0.423978, 0.423788, 0.423597, 0.423407, 
> 0.423216, 0.423026, 0.422836, 0.422646, 0.422456, 0.422266, 0.422076, 
> 0.421886,
> +0.421697
> +};
> +
> +#if !defined(min)
> +#define min(x, y) ((x) <= (y) ? (x) : (y))

Please use FFMIN()

> +#endif
> +
> +#define GAIN_APPLY(sample, gain) (sample) *= (gain)
> +
> +#define CHANNEL_NUM 2
> +
> +typedef struct {
> +uint64_t window;
> +unsigned char readahead, arg, control;
> +int running_gain;
> +unsigned sustain, sustain_reset;
> +int code_counterA;
> +int code_counterB;
> +int code_counterC;
> +} hdcd_state_t;
> +
> +typedef struct HDCDContext {
> +const AVClass *class;
> +hdcd_state_t state[CHANNEL_NUM];

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

> +} HDCDContext;
> +
> +
> +static const AVOption hdcd_options[] = {
> + {NULL}

Please remove this if its not going to be used.

> +};

New line.

> +AVFILTER_DEFINE_CLASS(hdcd);
> +
> +#define GAIN_APPLY(sample, gain) (sample) *= (gain)
> +
> +static void hdcd_reset(hdcd_state_t *state, unsigned rate) {

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

> +state->window = 0;
> +state->readahead = 32;
> +state->arg = 0;
> +state->control = 0;
> +
> +state->running_gain = 0;
> +
> +state->sustain = 0;
> +state->sustain_reset = rate * 10;
> +
> +state->code_counterA = 0;
> +state->code_counterB = 0;
> +state->code_counterC = 0;
> +}
> +

[...]

> +HDCDContext *s = ctx->priv;
> +
> +if (inlink->channels > CHANNEL_NUM) {
> +av_log(ctx, AV_LOG_ERROR, "HDCDs can have a maximum of %d 
> channels\n", CHANNEL_NUM);
> +return AVERROR(EINVAL);

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

> +}
> +
> +for (c = 0; c < inlink->channels; c++) {
> +hdcd_reset(>state[c], inlink->sample_rate);
> +}
> +
> +return 0;
> +}
> +
> +static const AVFilterPad avfilter_af_hdcd_inputs[] = {
> +{
> +.name = "default",
> +.type = AVMEDIA_TYPE_AUDIO,
> +.config_props = config_input,
> +.filter_frame = filter_frame,
> +},
> +{ NULL }
> +};
> +
> +static const AVFilterPad avfilter_af_hdcd_outputs[] = {
> +{
> +.name = "default",
> +.type = AVMEDIA_TYPE_AUDIO,
> +},
> +{ NULL }
> +};
> +
> +AVFilter ff_af_hdcd = {
> +.name  = "hdcd",
> +.description   = NULL_IF_CONFIG_SMALL("Apply hdcd decoding."),

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

> +.priv_size

Re: [FFmpeg-devel] [PATCH] Implement hdcd filtering

2016-03-22 Thread Carl Eugen Hoyos 
Benjamin St  gmail.com> writes:

> This patch applies filtering/decoding for hdcds(see ticket #4441) 

Great!

Please remember to make a proposal for your GSoC task on the 
Google site.

Carl Eugen

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel