diff mbox

[FFmpeg-devel] aacenc: WIP support for PCEs

Message ID 73537ed2-5482-0303-0a1f-1c21d4ff24a2@gmail.com
State New
Headers show

Commit Message

pkv.stream Nov. 9, 2017, 8:01 a.m. UTC 
Le 09/11/2017 à 4:43 AM, Rostislav Pehlivanov a écrit :
> On 18 October 2017 at 11:05, pkv.stream <pkv.stream@gmail.com> wrote:
>
>> Le 02/10/2017 à 8:39 PM, Rostislav Pehlivanov a écrit :
>>
>>> On 2 October 2017 at 18:43, pkv.stream <pkv.stream@gmail.com> wrote:
>>>
>>> Le 02/10/2017 à 7:23 PM, Michael Niedermayer a écrit :
>>>> On Mon, Oct 02, 2017 at 12:52:53AM +0200, pkv.stream wrote:
>>>>> Le 02/10/2017 à 12:43 AM, Carl Eugen Hoyos a écrit :
>>>>>> 2017-10-02 0:40 GMT+02:00 pkv.stream <pkv.stream@gmail.com>:
>>>>>>> Hi atomnuker,
>>>>>>>> got your PCE working;
>>>>>>>>
>>>>>>>> the patch you attached contains tabs, they cannot be committed
>>>>>>> to the FFmpeg repository, please remove them.
>>>>>>> (Or one tab.)
>>>>>>>
>>>>>>> thanks for pointing out.
>>>>>> Removed the offending tab.
>>>>>>
>>>>>> Thank you, Carl Eugen
>>>>>>
>>>>>>> _______________________________________________
>>>>>>> ffmpeg-devel mailing list
>>>>>>> ffmpeg-devel@ffmpeg.org
>>>>>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>>>>>
>>>>>>>     aacenc.h |  239 ++++++++++++++++++++++++++++++
>>>>>> +++++++++++++++++++++++++++++++--
>>>>>>     1 file changed, 233 insertions(+), 6 deletions(-)
>>>>>> 929275fe34af4d0048bac2be928957288cb75ddd
>>>>>> 0001-avcodec-aacenc-PCE-for-al
>>>>>> l-ffmpeg-usual-layouts.patch
>>>>>>    From 647fb61708bc1279f9dc17c679052a778dce5fbb Mon Sep 17 00:00:00
>>>>>> 2001
>>>>>> From: pkviet <pkv.stream@gmail.com>
>>>>>> Date: Sun, 24 Sep 2017 16:11:17 +0200
>>>>>> Subject: [PATCH] avcodec/aacenc: PCE for all ffmpeg usual layouts
>>>>>>
>>>>>> this seems not to apply cleanly here, did i miss something ?
>>>>> Hi Michael
>>>> this needs to be applied after the initial patch by atomnuker which he
>>>> did
>>>> not apply since this required work.
>>>> What i submitted was not aimed at being pushed since there is probably
>>>> still work to do.
>>>> Depending on what he wants to do with his patch, I'll resubmit a working
>>>> patch later, properly rebased.
>>>> Sorry about the mess.
>>>> regards
>>>>
>>>>
>>>> Applying: avcodec/aacenc: PCE for all ffmpeg usual layouts
>>>>> error: sha1 information is lacking or useless (libavcodec/aacenc.h).
>>>>> error: could not build fake ancestor
>>>>> Patch failed at 0001 avcodec/aacenc: PCE for all ffmpeg usual layouts
>>>>> The copy of the patch that failed is found in: .git/rebase-apply/patch
>>>>>
>>>>>
>>>>> [...]
>>>>>
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> ffmpeg-devel mailing list
>>>>> ffmpeg-devel@ffmpeg.org
>>>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>>>
>>>>>
>>>> _______________________________________________
>>>> ffmpeg-devel mailing list
>>>> ffmpeg-devel@ffmpeg.org
>>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>>
>>>>
>>> Give me a few hours and I'll test it and submit a v2 of my patch with your
>>> improvements.
>>> _______________________________________________
>>> ffmpeg-devel mailing list
>>> ffmpeg-devel@ffmpeg.org
>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>
>> Hi
>>
>> any updates ?
>>
>> regards
>>
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>
> Hi,
> Very sorry it took me this long but I finally got motivated and got around
> to checking your patch
>
> I have to say I'm impressed, everything works perfectly, decodes fine and
> the mappings were all fine. This is a big feature which many people have
> requested and complained the encoder lacks support for.
>
> I've done some minor changes to the code on the encoder side (an INFO print
> instead of a warning), to the comments (just alignment) and for the
> ambisonic layouts (made them use the defines) and I've pushed it.
>
> Thanks a lot
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Hi atomnuker,

that's wonderful;

there are two things also:

1) there are changes to make to the list of channel layouts not 
requiring PCE

==> AV_CH_LAYOUT_5POINT0 to AV_CH_LAYOUT_5POINT0_BACK since the previous 
is 5.0(side) while the latter is 5.0 which is what is in spec (table 
1.19 ISO/IEC 14496-3:200X(E) or table 42 ISO/IEC 13818-7:2004(E) )

see patch in attachment (can't be applied directly due to rebasing 
issues from your initial patch)

2) for everything to work I had to also apply the patch from here:

http://ffmpeg.org/pipermail/ffmpeg-devel/2017-October/217357.html

If you ffmpeg -loglevel debug , you will see that on non-default channel 
layouts, there is an auto insertion of a resampler filter : the 
channel_layout option is not passed correctly in the chain.

for instance: ffmpeg -channel_layout octagonal -i input.wav -c:a aac 
-channel_layout octagonal out.mkv will matrix the input from octagonal 
to 7.1 before the encoding.

Check ticket 6706 for details of the issue.

I am not knowledgeable enough to be sure my fix is correct; it's working 
for sure, but I've had very few feedback (only Michael and Moritz about 
styling issues).

Maybe you could have a look.

Regards

pkv
From 847334071ff13a00d54b9f55ccf29c7553ab83f1 Mon Sep 17 00:00:00 2001
From: pkviet <pkv.stream@gmail.com>
Date: Mon, 6 Nov 2017 10:03:47 +0100
Subject: [PATCH] avcodec/aacenc: fix default channels

Fix default channels not requiring PCE. Replaces 5.0 and 5.1 definitions; 7.1 should be AV_CH_LAYOUT_7POINT1_WIDE_BACK acording to spec but this is not used in practice, so keep AV_CH_LAYOUT_7POINT1 which is what is used widely.
---
 libavcodec/aacenctab.h | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Comments

Carl Eugen Hoyos Nov. 14, 2017, 12:20 a.m. UTC | #1 
2017-11-09 9:01 GMT+01:00 pkv.stream <pkv.stream@gmail.com>:

> 1) there are changes to make to the list of channel layouts not requiring
> PCE
>
> ==> AV_CH_LAYOUT_5POINT0 to AV_CH_LAYOUT_5POINT0_BACK since the previous is
> 5.0(side) while the latter is 5.0 which is what is in spec (table 1.19
> ISO/IEC 14496-3:200X(E) or table 42 ISO/IEC 13818-7:2004(E) )
>
> see patch in attachment (can't be applied directly due to rebasing issues
> from your initial patch)

Did you test what effect this patch has on ffmpeg (the command line interface)
when encoding five- or six-channel aac from back and side source?
If something weird happens (like resampling), a Changelog entry may be a
good idea.

Thank you, Carl Eugen
pkv.stream Nov. 14, 2017, 8:01 p.m. UTC | #2 
Le 14/11/2017 à 1:20 AM, Carl Eugen Hoyos a écrit :
> 2017-11-09 9:01 GMT+01:00 pkv.stream <pkv.stream@gmail.com>:
>
>> 1) there are changes to make to the list of channel layouts not requiring
>> PCE
>>
>> ==> AV_CH_LAYOUT_5POINT0 to AV_CH_LAYOUT_5POINT0_BACK since the previous is
>> 5.0(side) while the latter is 5.0 which is what is in spec (table 1.19
>> ISO/IEC 14496-3:200X(E) or table 42 ISO/IEC 13818-7:2004(E) )
>>
>> see patch in attachment (can't be applied directly due to rebasing issues
>> from your initial patch)
> Did you test what effect this patch has on ffmpeg (the command line interface)
> when encoding five- or six-channel aac from back and side source?
yes I did check, this patch reverts aac encoder to previous behaviour 
with 5.0 and 5.1 (when there was no PCE)

> If something weird happens (like resampling), a Changelog entry may be a
> good idea.

>
> Thank you, Carl Eugen
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Carl Eugen Hoyos Nov. 14, 2017, 10:14 p.m. UTC | #3 
2017-11-14 21:01 GMT+01:00 pkv.stream <pkv.stream@gmail.com>:
> Le 14/11/2017 à 1:20 AM, Carl Eugen Hoyos a écrit :
>>
>> 2017-11-09 9:01 GMT+01:00 pkv.stream <pkv.stream@gmail.com>:
>>
>>> 1) there are changes to make to the list of channel layouts not requiring
>>> PCE
>>>
>>> ==> AV_CH_LAYOUT_5POINT0 to AV_CH_LAYOUT_5POINT0_BACK
>>> since the previous is
>>> 5.0(side) while the latter is 5.0 which is what is in spec (table 1.19
>>> ISO/IEC 14496-3:200X(E) or table 42 ISO/IEC 13818-7:2004(E) )
>>>
>>> see patch in attachment (can't be applied directly due to rebasing issues
>>> from your initial patch)
>>
>> Did you test what effect this patch has on ffmpeg (the command line
>> interface)
>> when encoding five- or six-channel aac from back and side source?
>
> yes I did check, this patch reverts aac encoder to previous behaviour with
> 5.0 and 5.1 (when there was no PCE)

Thank you for the explanation!

Carl Eugen
pkv.stream Nov. 14, 2017, 10:38 p.m. UTC | #4 
Le 14/11/2017 à 11:14 PM, Carl Eugen Hoyos a écrit :
> 2017-11-14 21:01 GMT+01:00 pkv.stream <pkv.stream@gmail.com>:
>> Le 14/11/2017 à 1:20 AM, Carl Eugen Hoyos a écrit :
>>> 2017-11-09 9:01 GMT+01:00 pkv.stream <pkv.stream@gmail.com>:
>>>
>>>> 1) there are changes to make to the list of channel layouts not requiring
>>>> PCE
>>>>
>>>> ==> AV_CH_LAYOUT_5POINT0 to AV_CH_LAYOUT_5POINT0_BACK
>>>> since the previous is
>>>> 5.0(side) while the latter is 5.0 which is what is in spec (table 1.19
>>>> ISO/IEC 14496-3:200X(E) or table 42 ISO/IEC 13818-7:2004(E) )
>>>>
>>>> see patch in attachment (can't be applied directly due to rebasing issues
>>>> from your initial patch)
>>> Did you test what effect this patch has on ffmpeg (the command line
>>> interface)
>>> when encoding five- or six-channel aac from back and side source?
>> yes I did check, this patch reverts aac encoder to previous behaviour with
>> 5.0 and 5.1 (when there was no PCE)

more accurately: 5.0 5.1 back is reverted to previous behaviour  , but 
5.0 5.1 side is not because it uses now PCE, which agrees with spec (no 
weird channel rematrixing though); sorry for the inaccuracy.

> Thank you for the explanation!
>
> Carl Eugen
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
diff mbox

Patch

diff --git a/libavcodec/aacenctab.h b/libavcodec/aacenctab.h
index c852a29..ab1023c 100644
--- a/libavcodec/aacenctab.h
+++ b/libavcodec/aacenctab.h
@@ -45,13 +45,13 @@  extern const int      ff_aac_swb_size_128_len;
 
 /* Supported layouts without using a PCE */
 static const int64_t aac_normal_chan_layouts[7] = {
-    AV_CH_LAYOUT_MONO,
-    AV_CH_LAYOUT_STEREO,
-    AV_CH_LAYOUT_SURROUND,
-    AV_CH_LAYOUT_4POINT0,
-    AV_CH_LAYOUT_5POINT0,
-    AV_CH_LAYOUT_5POINT1,
-    AV_CH_LAYOUT_7POINT1,
+    AV_CH_LAYOUT_MONO,
+    AV_CH_LAYOUT_STEREO,
+    AV_CH_LAYOUT_SURROUND,
+    AV_CH_LAYOUT_4POINT0,
+    AV_CH_LAYOUT_5POINT0_BACK, // -channel_layout 5.0
+    AV_CH_LAYOUT_5POINT1_BACK, //  -channel_layout 5.1
+    AV_CH_LAYOUT_7POINT1, //  should be AV_CH_LAYOUT_7POINT1_WIDE_BACK acording to spec but unpractical
 };
 
 /** default channel configurations */