[FFmpeg-devel] doc/decoders: Clear up description of ac3's drc_scale option

Make clear that the value to -drc_scale is an exponentiating value, not
a scaling factor. Also, document the default value.

Signed-off-by: Aman Verma <amanraoverma@gmail.com>
---
 doc/decoders.texi | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

On 2020-08-28 15:39, Aman Verma wrote:
> Make clear that the value to -drc_scale is an exponentiating value, not
> a scaling factor. Also, document the default value.
>
> Signed-off-by: Aman Verma <amanraoverma@gmail.com>
> ---
>   doc/decoders.texi | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/doc/decoders.texi b/doc/decoders.texi
> index 9005714..4a307b4 100644
> --- a/doc/decoders.texi
> +++ b/doc/decoders.texi
> @@ -106,8 +106,8 @@ the undocumented RealAudio 3 (a.k.a. dnet).
>   @table @option
>   
>   @item -drc_scale @var{value}
> -Dynamic Range Scale Factor. The factor to apply to dynamic range values
> -from the AC-3 stream. This factor is applied exponentially.
> +Dynamic Range Compression scale. The number to apply---exponentially---to
> +dynamic range values from the AC-3 stream. The default value is 1.
>   There are 3 notable scale factor ranges:
>   @table @option
>   @item drc_scale == 0

Aman, thank you for working on this patch.

I don't know anything about the AC-3 format or the ac3 decoder in 
FFmpeg. However, I can read technical writing. I don't understand what 
you are trying to say differently with this change. But even more, I 
don't understand what the existing documentation is trying to say.

I like that you are adding the information that the default value of 
-drc_scale is 1. That is not presently documented. Good.

It looks like the other changes are to change "factor" to "number", and  
to convert a sentence "This factor is applied exponentially." to a 
dashed phrase "---exponentially---". I don't feel strongly that this 
makes things worth, but I also don't see how this makes things clearer.

However, both the old and the new text leave me with a lot of questions. 
No doubt some of my questions are due to my ignorance of the AC-3 format 
and of the ac3 decoder code in FFmpeg. But some are due to the text 
leaving a lot of information unstated.

What does it mean to "apply — exponentially — to" a number? What 
mathematical operation is that? This is not at all clear to me.

Does the term "dynamic range values from the AC-3 stream" refer to the 
`dynrng` and `dynrng2` values as defined in section 7.7.1 "Dynamic Range 
Control" of A/52:2012, "Digital Audio Compression (AC-3, E-AC-3)"[1]? 
Then using the names of the values as defined in the specification will 
be more precise.

The `dynrng` and `dynrng2` values each hold a 3-bit value and a 5-bit 
value. The AC-3 specification says that the decoder interprets the 3-bit 
value as gain changes from -18.06 dB to +24.08 dB, and the 5-bit value 
as linear gain changes from –0.14 dB to –6.02 dB. (Section 7.7.1.2 pp 
88-89.)

Does the ac3 decoder do a computation on the `dynrng` and `dynrng2` 
values? Does it do a computation on the gain change value derived from 
the `dynrng` and `dynrng2` values? What is this computation? Or, does it 
use the -drc_scale value in place of the `dynrng` and `dynrng2` values 
in the audio stream?

The further description of the -drc_scale value describes three 
behaviours, based on the -drc_scale value. It says, "drc_scale == 0. DRC 
disabled. Produces full range audio." Does this mean that the ac3 
decoder uses the `dynrng` and `dynrng2` values unchanged, or that it 
decodes as if those values were zero?

And, what does DRC refer to in this sentence? Does it mean the 
adjustment made by the ac3 decoder based on the -drc_scale value? Or 
does it mean Dynamic Range Compression as described in section 7.7 of 
the A/52:2012 specification?

The description continues, "0 < drc_scale <= 1. DRC enabled. Applies a 
fraction of the stream DRC value. Audio reproduction is between full 
range and full compression." What decoder behaviour does "DRC enabled" 
mean?  What does  "Applies a fraction of the stream DRC value" mean? 
What does the applying? What does it apply to?  Does it mean the ac3 
decoder does an arithmetic operation between the -drc_scale value passed 
to the decoder, and the `dynrng` and `dynrng2` values from the ac3 
stream?  What is this operation?  And what does "Audio reproduction is 
between full range and full compression." mean? Does it indicate a 
different modification which the decoder makes to the decoded audio 
signal? Or is it explaining the effect on the decoded audio signal of 
how the decoder interprets the result of the arithmetic operation?

The description continues, "drc_scale > 1. DRC enabled. Applies 
drc_scale asymmetrically. Loud sounds are fully compressed. Soft sounds 
are enhanced."  What does "Applies drc_scale asymmetrically." mean? What 
is the arithmetic operation?  What values, what range does the decoder 
treat "asymmetrically"? What is the shape of this asymmetry? What does 
"fully compressed" mean — made silent? Reduced to median loudness? 
Something else?  What does "Soft sounds are enhanced." mean?  Made 
louder?  How much louder?

[1] https://www.atsc.org/wp-content/uploads/2015/03/A52-201212-17.pdf

So, I'm not in a position to approve or reject this patch.  And fixing 
deficiencies in the ac3 decoder documentation overall is probably not 
the scope of change which you wanted to make. But I think this ac3 
decoder documentation could stand to be improved a lot. If you have just 
been looking at the code, perhaps you well informed to answer these 
questions.

On Fri, Aug 28, 2020 at 11:15 PM Jim DeLaHunt <list+ffmpeg-dev@jdlh.com> wrote:
> I don't know anything about the AC-3 format or the ac3 decoder in
> FFmpeg. However, I can read technical writing. I don't understand what
> you are trying to say differently with this change. But even more, I
> don't understand what the existing documentation is trying to say.
> I like that you are adding the information that the default value of
> -drc_scale is 1. That is not presently documented. Good.
>
> It looks like the other changes are to change "factor" to "number", and
> to convert a sentence "This factor is applied exponentially." to a
> dashed phrase "---exponentially---". I don't feel strongly that this
> makes things worth, but I also don't see how this makes things clearer.
>
> However, both the old and the new text leave me with a lot of questions.
> No doubt some of my questions are due to my ignorance of the AC-3 format
> and of the ac3 decoder code in FFmpeg. But some are due to the text
> leaving a lot of information unstated.

That is a fair judgement, I mostly reworded it because I didn't like the
sentence structure and I didn't look too closely at the source beyond
finding the default value. Looking at it now, it seems that the comments
in ac3dec_fixed.c, ac3dec_float.c, and ac3dec.h claim that the value is
applied as a linear scaler, contradicting the claims in the CLI
documentation [1][2][3].

> What does it mean to "apply — exponentially — to" a number? What
> mathematical operation is that? This is not at all clear to me.

That definitely could have been made clearer haha. I was referring to a
function f(x,d) = x^d, where x is the number and d is the drc_scale
value. I think that is also what the original documentation meant.

> So, I'm not in a position to approve or reject this patch.  And fixing
> deficiencies in the ac3 decoder documentation overall is probably not
> the scope of change which you wanted to make.

For now, I'll revise the patch to only include the mention of the
default value but I will do a closer reading of the ac3dec source when I
have more time.

As an aside, am I supposed to submit the revised patch in reply to this
thread or should I submit it as a new thread? This is my first time
contributing to a project that uses mailing lists.

[1]: https://github.com/FFmpeg/FFmpeg/blob/5ff2ff6bd9cd9e08729060d330e381a09972c498/libavcodec/ac3dec_fixed.c#L159
[2]: https://github.com/FFmpeg/FFmpeg/blob/5ff2ff6bd9cd9e08729060d330e381a09972c498/libavcodec/ac3dec_float.c#L36
[3]: https://github.com/FFmpeg/FFmpeg/blob/5ff2ff6bd9cd9e08729060d330e381a09972c498/libavcodec/ac3dec.h#L175

On 2020-08-29 15:10, Aman Verma wrote:

> ...As an aside, am I supposed to submit the revised patch in reply to 
> this
> thread or should I submit it as a new thread? This is my first time
> contributing to a project that uses mailing lists.

The instructions are at 
<http://ffmpeg.org/developer.html#Submitting-patches-1>, sections 6 
"Submitting patches" and 8 "Patch submission checklist". I found those 
instructions a bit of a jumble and hard to follow, so I just reread them 
all every step of the way. As far as I can tell they don't actually 
answer your question. But it is a common practice to use "git patch 
--in-reply-to=<message id>" to set the "In-reply-to" header to the 
message-ID of your original patch email.

You still need to have a review from someone with commit authority, 
which is not me. Good luck!

On 30-08-2020 05:18 am, Jim DeLaHunt wrote:
> On 2020-08-29 15:10, Aman Verma wrote:
>
>> ...As an aside, am I supposed to submit the revised patch in reply to 
>> this
>> thread or should I submit it as a new thread? This is my first time
>> contributing to a project that uses mailing lists.
>
> The instructions are at 
> <http://ffmpeg.org/developer.html#Submitting-patches-1>, sections 6 
> "Submitting patches" and 8 "Patch submission checklist". I found those 
> instructions a bit of a jumble and hard to follow, so I just reread 
> them all every step of the way. As far as I can tell they don't 
> actually answer your question. But it is a common practice to use "git 
> patch --in-reply-to=<message id>" to set the "In-reply-to" header to 
> the message-ID of your original patch email.
>
> You still need to have a review from someone with commit authority, 
> which is not me. Good luck!

Submit a new patch with updated version number, and reply to using the 
message-id of the parent email.

Gyan