diff mbox

[FFmpeg-devel] CNG (consistent noise generation) patch for AC-3 decoder

Message ID 57CA1022.9050304@impactstudiopro.com
State Superseded
Headers show

Commit Message

Jonathan Campbell Sept. 2, 2016, 11:49 p.m. UTC
On 09/02/2016 04:37 PM, Michael Niedermayer wrote:
> On Fri, Sep 02, 2016 at 04:05:44PM -0700, Jonathan Campbell wrote:
>> On 09/02/2016 01:56 PM, Michael Niedermayer wrote:
>>> On Fri, Sep 02, 2016 at 01:13:33PM -0700, Jonathan Campbell wrote:
>>>> On 09/02/2016 01:01 PM, Michael Niedermayer wrote:
>>>>> On Fri, Sep 02, 2016 at 10:19:23AM -0700, Jonathan Campbell wrote:
>>>>> [...]
>>>>>> CRC computation isn't fast enough? What should I use then? A sum of
>>>>>> byte values?
>>>>> av_lfg_init() calls av_md5_sum()
>>>>> av_md5_sum() is too slow to be called per ac3 frame
>>>>>
>>>>> [...]
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> ffmpeg-devel mailing list
>>>>> ffmpeg-devel@ffmpeg.org
>>>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>> Then for this to work at better performance, I'm going to have to
>>>> either directly modify the AVLFG struct state (probably not a good
>>>> idea), or add a function to libavutil/lfg.c that allows "fast
>>>> seeding" without the use of MD5 which could probably be something as
>>>> simple as copy the 32-bit seed 16 times through c->state[] with or
>>>> without modification during the loop. Sound good?
>>> yes, something like this
>>> probably spliting the input into 16 parts get 16 CRCs and use them
>>> instead of replication is probably better
>>>
>>>
>>> [...]
>>>
>>>
>>> _______________________________________________
>>> ffmpeg-devel mailing list
>>> ffmpeg-devel@ffmpeg.org
>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> How's this:
>>
>> void av_lfg_init_from_data(AVLFG *c, const unsigned char *data,
>> unsigned int length) {
>>      unsigned int beg, end, segm;
>>      const AVCRC *avcrc;
>>      uint32_t crc = 0;
> 1 might be better
>
> should be ok otherwise didnt review it deeply though
>
> [...]
>
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Finished patch. Hopefully it won't get garbled as an attachment.

Jonathan Campbell

Comments

Michael Niedermayer Sept. 3, 2016, 12:23 a.m. UTC | #1
On Fri, Sep 02, 2016 at 04:49:54PM -0700, Jonathan Campbell wrote:
> 
> On 09/02/2016 04:37 PM, Michael Niedermayer wrote:
> >On Fri, Sep 02, 2016 at 04:05:44PM -0700, Jonathan Campbell wrote:
> >>On 09/02/2016 01:56 PM, Michael Niedermayer wrote:
> >>>On Fri, Sep 02, 2016 at 01:13:33PM -0700, Jonathan Campbell wrote:
> >>>>On 09/02/2016 01:01 PM, Michael Niedermayer wrote:
> >>>>>On Fri, Sep 02, 2016 at 10:19:23AM -0700, Jonathan Campbell wrote:
> >>>>>[...]
> >>>>>>CRC computation isn't fast enough? What should I use then? A sum of
> >>>>>>byte values?
> >>>>>av_lfg_init() calls av_md5_sum()
> >>>>>av_md5_sum() is too slow to be called per ac3 frame
> >>>>>
> >>>>>[...]
> >>>>>
> >>>>>
> >>>>>_______________________________________________
> >>>>>ffmpeg-devel mailing list
> >>>>>ffmpeg-devel@ffmpeg.org
> >>>>>http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >>>>Then for this to work at better performance, I'm going to have to
> >>>>either directly modify the AVLFG struct state (probably not a good
> >>>>idea), or add a function to libavutil/lfg.c that allows "fast
> >>>>seeding" without the use of MD5 which could probably be something as
> >>>>simple as copy the 32-bit seed 16 times through c->state[] with or
> >>>>without modification during the loop. Sound good?
> >>>yes, something like this
> >>>probably spliting the input into 16 parts get 16 CRCs and use them
> >>>instead of replication is probably better
> >>>
> >>>
> >>>[...]
> >>>
> >>>
> >>>_______________________________________________
> >>>ffmpeg-devel mailing list
> >>>ffmpeg-devel@ffmpeg.org
> >>>http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >>How's this:
> >>
> >>void av_lfg_init_from_data(AVLFG *c, const unsigned char *data,
> >>unsigned int length) {
> >>     unsigned int beg, end, segm;
> >>     const AVCRC *avcrc;
> >>     uint32_t crc = 0;
> >1 might be better
> >
> >should be ok otherwise didnt review it deeply though
> >
> >[...]
> >
> >
> >_______________________________________________
> >ffmpeg-devel mailing list
> >ffmpeg-devel@ffmpeg.org
> >http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> Finished patch. Hopefully it won't get garbled as an attachment.
> 
> Jonathan Campbell
> 

>  libavcodec/ac3dec.c       |    7 +++++++
>  libavcodec/ac3dec.h       |    4 ++++
>  libavcodec/ac3dec_fixed.c |    1 +
>  libavcodec/ac3dec_float.c |    1 +

>  libavutil/lfg.c           |   27 +++++++++++++++++++++++++++
>  libavutil/lfg.h           |    5 +++++

should be in a different patch
also needs minor version bump and APIChanges update

also please generate patches with git format-patch or git send-email
see the respective man pages
(otherwise they woul be lacking commit messages)

thx

[...]
Jonathan Campbell Sept. 3, 2016, 12:31 a.m. UTC | #2
> should be in a different patch
> also needs minor version bump and APIChanges update
>
> also please generate patches with git format-patch or git send-email
> see the respective man pages
> (otherwise they woul be lacking commit messages)
>
> thx
>
> [...]
>
>
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
One patch for the new function in lfg.c, and another patch to use the 
addition in the ac3 decoder, right? I can do that.

The version minor bump would bring libavutil 55.30.100 (or should it be 
55.30.0?). I'll format the patch correctly.

Jonathan Campbell
James Almer Sept. 3, 2016, 12:34 a.m. UTC | #3
On 9/2/2016 8:49 PM, Jonathan Campbell wrote:
> diff --git a/libavcodec/ac3dec.c b/libavcodec/ac3dec.c
> index fac189b..18a674b 100644
> --- a/libavcodec/ac3dec.c
> +++ b/libavcodec/ac3dec.c
> @@ -1419,6 +1419,13 @@ static int ac3_decode_frame(AVCodecContext * avctx, void *data,
>                              (const uint16_t *) buf, cnt);
>      } else
>          memcpy(s->input_buffer, buf, FFMIN(buf_size, AC3_FRAME_BUFFER_SIZE));
> +
> +    /* if consistent noise generation is enabled, seed the linear feedback generator
> +     * with the contents of the AC-3 frame so that the noise is identical across
> +     * decodes given the same AC-3 frame data, for use with non-linear edititing software. */
> +    if (s->consistent_noise_generation)
> +        av_lfg_init_from_data(&s->dith_state, s->input_buffer, FFMIN(buf_size, AC3_FRAME_BUFFER_SIZE));
> +
>      buf = s->input_buffer;
>      /* initialize the GetBitContext with the start of valid AC-3 Frame */
>      if ((ret = init_get_bits8(&s->gbc, buf, buf_size)) < 0)
> diff --git a/libavcodec/ac3dec.h b/libavcodec/ac3dec.h
> index c2b867e..98184e9 100644
> --- a/libavcodec/ac3dec.h
> +++ b/libavcodec/ac3dec.h
> @@ -177,6 +177,10 @@ typedef struct AC3DecodeContext {
>      int end_freq[AC3_MAX_CHANNELS];         ///< end frequency bin                      (endmant)
>  ///@}
>  
> +///@name Consistent noise generation
> +    int consistent_noise_generation;        ///< seed noise generation with AC-3 frame on decode
> +///@}
> +
>  ///@name Rematrixing
>      int num_rematrixing_bands;              ///< number of rematrixing bands            (nrematbnd)
>      int rematrixing_flags[4];               ///< rematrixing flags                      (rematflg)
> diff --git a/libavcodec/ac3dec_fixed.c b/libavcodec/ac3dec_fixed.c
> index 6416da4..1f79ade 100644
> --- a/libavcodec/ac3dec_fixed.c
> +++ b/libavcodec/ac3dec_fixed.c
> @@ -168,6 +168,7 @@ static void ac3_downmix_c_fixed16(int16_t **samples, int16_t (*matrix)[2],
>  #include "ac3dec.c"
>  
>  static const AVOption options[] = {
> +    { "cons_noisegen", "enable consistent noise generation", OFFSET(consistent_noise_generation), AV_OPT_TYPE_BOOL, {.i64 = 0 }, 0, 1, PAR },
>      { "drc_scale", "percentage of dynamic range compression to apply", OFFSET(drc_scale), AV_OPT_TYPE_FLOAT, {.dbl = 1.0}, 0.0, 6.0, PAR },
>      { "heavy_compr", "enable heavy dynamic range compression", OFFSET(heavy_compression), AV_OPT_TYPE_BOOL, {.i64 = 0 }, 0, 1, PAR },
>      { NULL},
> diff --git a/libavcodec/ac3dec_float.c b/libavcodec/ac3dec_float.c
> index 0a5319a..b85a4ce 100644
> --- a/libavcodec/ac3dec_float.c
> +++ b/libavcodec/ac3dec_float.c
> @@ -32,6 +32,7 @@
>  #include "ac3dec.c"
>  
>  static const AVOption options[] = {
> +    { "cons_noisegen", "enable consistent noise generation", OFFSET(consistent_noise_generation), AV_OPT_TYPE_BOOL, {.i64 = 0 }, 0, 1, PAR },
>      { "drc_scale", "percentage of dynamic range compression to apply", OFFSET(drc_scale), AV_OPT_TYPE_FLOAT, {.dbl = 1.0}, 0.0, 6.0, PAR },
>      { "heavy_compr", "enable heavy dynamic range compression", OFFSET(heavy_compression), AV_OPT_TYPE_BOOL, {.i64 = 0 }, 0, 1, PAR },
>      { "target_level", "target level in -dBFS (0 not applied)", OFFSET(target_level), AV_OPT_TYPE_INT, {.i64 = 0 }, -31, 0, PAR },

Bump libavcodec micro version by 1 in its own patch as Michael requested.

> diff --git a/libavutil/lfg.c b/libavutil/lfg.c
> index 08a4f67..178d27e 100644
> --- a/libavutil/lfg.c
> +++ b/libavutil/lfg.c
> @@ -23,6 +23,7 @@
>  #include <limits.h>
>  #include <math.h>
>  #include "lfg.h"
> +#include "crc.h"
>  #include "md5.h"
>  #include "intreadwrite.h"
>  #include "attributes.h"
> @@ -58,3 +59,29 @@ void av_bmg_get(AVLFG *lfg, double out[2])
>      out[0] = x1 * w;
>      out[1] = x2 * w;
>  }
> +
> +void av_lfg_init_from_data(AVLFG *c, const unsigned char *data, unsigned int length) {

Don't return void if one of the arguments can be invalid. The user needs to know
if init failed in some way. Return int.
Also, crc (and every other hash function) expect const uint8_t*, so use that for data.

> +    unsigned int beg, end, segm;
> +    const AVCRC *avcrc;
> +    uint32_t crc = 1;
> +
> +    c->index = 0;
> +
> +    avcrc = av_crc_get_table(AV_CRC_32_IEEE);

This can't fail. It's a well defined table in crc.c, so just remove the check below.

> +    if (avcrc == NULL) return;
> +
> +    /* try to avoid integer overflow during the segmented crc loop below.
> +     * the code below would break if "end" went backwards before "beg". */
> +    if (length > (UINT_MAX / 128U)) return;

return AVERROR(EINVAL).

> +
> +    /* across 64 pieces of the incoming data,
> +     * do a running crc of each segment and store the crc as the state for that slot.
> +     * this works even if the length of the piece is 0 bytes. */
> +    beg = 0;
> +    for (segm = 0;segm < 64;segm++) {
> +        end = (((segm + 1) * length) / 64);
> +        crc = av_crc(avcrc, crc, data + beg, end - beg);
> +        c->state[segm] = (unsigned int)crc;
> +        beg = end;
> +    }

return 0 on success, of course.

> +}
> diff --git a/libavutil/lfg.h b/libavutil/lfg.h
> index ec90562..f3c08ec 100644
> --- a/libavutil/lfg.h
> +++ b/libavutil/lfg.h
> @@ -29,6 +29,11 @@ typedef struct AVLFG {
>  
>  void av_lfg_init(AVLFG *c, unsigned int seed);
>  
> +/*
> + * Seed the state of the ALFG using binary data
> + */

Make sure to mention possible return values are 0 on success, negative value on
failure after the above changes.

> +void av_lfg_init_from_data(AVLFG *c, const unsigned char *data, unsigned int length);
> +
>  /**
>   * Get the next random unsigned 32-bit number using an ALFG.
>   *

You should add a line in doc/APIChanges about the new function, and bump
minor version for libavutil as Michael requested.
Michael Niedermayer Sept. 3, 2016, 12:35 a.m. UTC | #4
On Fri, Sep 02, 2016 at 05:31:35PM -0700, Jonathan Campbell wrote:
> >should be in a different patch
> >also needs minor version bump and APIChanges update
> >
> >also please generate patches with git format-patch or git send-email
> >see the respective man pages
> >(otherwise they woul be lacking commit messages)
> >
> >thx
> >
> >[...]
> >
> >
> >
> >_______________________________________________
> >ffmpeg-devel mailing list
> >ffmpeg-devel@ffmpeg.org
> >http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

> One patch for the new function in lfg.c, and another patch to use
> the addition in the ac3 decoder, right? I can do that.

yes


> 
> The version minor bump would bring libavutil 55.30.100 (or should it
> be 55.30.0?). I'll format the patch correctly.

55.30.100

thx

[...]
James Almer Sept. 3, 2016, 12:36 a.m. UTC | #5
On 9/2/2016 9:31 PM, Jonathan Campbell wrote:
>> should be in a different patch
>> also needs minor version bump and APIChanges update
>>
>> also please generate patches with git format-patch or git send-email
>> see the respective man pages
>> (otherwise they woul be lacking commit messages)
>>
>> thx
>>
>> [...]
>>
>>
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> One patch for the new function in lfg.c, and another patch to use the addition in the ac3 decoder, right? I can do that.
> 
> The version minor bump would bring libavutil 55.30.100 (or should it be 55.30.0?). I'll format the patch correctly.

The former. Micro version always starts at 100 for all the libraries.
See the doxy in libavutil/version.h for a full explanation of how it works.

> 
> Jonathan Campbell
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
diff mbox

Patch

diff --git a/libavcodec/ac3dec.c b/libavcodec/ac3dec.c
index fac189b..18a674b 100644
--- a/libavcodec/ac3dec.c
+++ b/libavcodec/ac3dec.c
@@ -1419,6 +1419,13 @@  static int ac3_decode_frame(AVCodecContext * avctx, void *data,
                             (const uint16_t *) buf, cnt);
     } else
         memcpy(s->input_buffer, buf, FFMIN(buf_size, AC3_FRAME_BUFFER_SIZE));
+
+    /* if consistent noise generation is enabled, seed the linear feedback generator
+     * with the contents of the AC-3 frame so that the noise is identical across
+     * decodes given the same AC-3 frame data, for use with non-linear edititing software. */
+    if (s->consistent_noise_generation)
+        av_lfg_init_from_data(&s->dith_state, s->input_buffer, FFMIN(buf_size, AC3_FRAME_BUFFER_SIZE));
+
     buf = s->input_buffer;
     /* initialize the GetBitContext with the start of valid AC-3 Frame */
     if ((ret = init_get_bits8(&s->gbc, buf, buf_size)) < 0)
diff --git a/libavcodec/ac3dec.h b/libavcodec/ac3dec.h
index c2b867e..98184e9 100644
--- a/libavcodec/ac3dec.h
+++ b/libavcodec/ac3dec.h
@@ -177,6 +177,10 @@  typedef struct AC3DecodeContext {
     int end_freq[AC3_MAX_CHANNELS];         ///< end frequency bin                      (endmant)
 ///@}
 
+///@name Consistent noise generation
+    int consistent_noise_generation;        ///< seed noise generation with AC-3 frame on decode
+///@}
+
 ///@name Rematrixing
     int num_rematrixing_bands;              ///< number of rematrixing bands            (nrematbnd)
     int rematrixing_flags[4];               ///< rematrixing flags                      (rematflg)
diff --git a/libavcodec/ac3dec_fixed.c b/libavcodec/ac3dec_fixed.c
index 6416da4..1f79ade 100644
--- a/libavcodec/ac3dec_fixed.c
+++ b/libavcodec/ac3dec_fixed.c
@@ -168,6 +168,7 @@  static void ac3_downmix_c_fixed16(int16_t **samples, int16_t (*matrix)[2],
 #include "ac3dec.c"
 
 static const AVOption options[] = {
+    { "cons_noisegen", "enable consistent noise generation", OFFSET(consistent_noise_generation), AV_OPT_TYPE_BOOL, {.i64 = 0 }, 0, 1, PAR },
     { "drc_scale", "percentage of dynamic range compression to apply", OFFSET(drc_scale), AV_OPT_TYPE_FLOAT, {.dbl = 1.0}, 0.0, 6.0, PAR },
     { "heavy_compr", "enable heavy dynamic range compression", OFFSET(heavy_compression), AV_OPT_TYPE_BOOL, {.i64 = 0 }, 0, 1, PAR },
     { NULL},
diff --git a/libavcodec/ac3dec_float.c b/libavcodec/ac3dec_float.c
index 0a5319a..b85a4ce 100644
--- a/libavcodec/ac3dec_float.c
+++ b/libavcodec/ac3dec_float.c
@@ -32,6 +32,7 @@ 
 #include "ac3dec.c"
 
 static const AVOption options[] = {
+    { "cons_noisegen", "enable consistent noise generation", OFFSET(consistent_noise_generation), AV_OPT_TYPE_BOOL, {.i64 = 0 }, 0, 1, PAR },
     { "drc_scale", "percentage of dynamic range compression to apply", OFFSET(drc_scale), AV_OPT_TYPE_FLOAT, {.dbl = 1.0}, 0.0, 6.0, PAR },
     { "heavy_compr", "enable heavy dynamic range compression", OFFSET(heavy_compression), AV_OPT_TYPE_BOOL, {.i64 = 0 }, 0, 1, PAR },
     { "target_level", "target level in -dBFS (0 not applied)", OFFSET(target_level), AV_OPT_TYPE_INT, {.i64 = 0 }, -31, 0, PAR },
diff --git a/libavutil/lfg.c b/libavutil/lfg.c
index 08a4f67..178d27e 100644
--- a/libavutil/lfg.c
+++ b/libavutil/lfg.c
@@ -23,6 +23,7 @@ 
 #include <limits.h>
 #include <math.h>
 #include "lfg.h"
+#include "crc.h"
 #include "md5.h"
 #include "intreadwrite.h"
 #include "attributes.h"
@@ -58,3 +59,29 @@  void av_bmg_get(AVLFG *lfg, double out[2])
     out[0] = x1 * w;
     out[1] = x2 * w;
 }
+
+void av_lfg_init_from_data(AVLFG *c, const unsigned char *data, unsigned int length) {
+    unsigned int beg, end, segm;
+    const AVCRC *avcrc;
+    uint32_t crc = 1;
+
+    c->index = 0;
+
+    avcrc = av_crc_get_table(AV_CRC_32_IEEE);
+    if (avcrc == NULL) return;
+
+    /* try to avoid integer overflow during the segmented crc loop below.
+     * the code below would break if "end" went backwards before "beg". */
+    if (length > (UINT_MAX / 128U)) return;
+
+    /* across 64 pieces of the incoming data,
+     * do a running crc of each segment and store the crc as the state for that slot.
+     * this works even if the length of the piece is 0 bytes. */
+    beg = 0;
+    for (segm = 0;segm < 64;segm++) {
+        end = (((segm + 1) * length) / 64);
+        crc = av_crc(avcrc, crc, data + beg, end - beg);
+        c->state[segm] = (unsigned int)crc;
+        beg = end;
+    }
+}
diff --git a/libavutil/lfg.h b/libavutil/lfg.h
index ec90562..f3c08ec 100644
--- a/libavutil/lfg.h
+++ b/libavutil/lfg.h
@@ -29,6 +29,11 @@  typedef struct AVLFG {
 
 void av_lfg_init(AVLFG *c, unsigned int seed);
 
+/*
+ * Seed the state of the ALFG using binary data
+ */
+void av_lfg_init_from_data(AVLFG *c, const unsigned char *data, unsigned int length);
+
 /**
  * Get the next random unsigned 32-bit number using an ALFG.
  *