[FFmpeg-devel] avcodec/aacenc: set pce value by options pce

Submitted by Steven Liu on Nov. 2, 2018, 9:16 a.m.

Details

Message ID 20181102091634.66159-1-lq@chinaffmpeg.org
State New
Headers show

Commit Message

Steven Liu Nov. 2, 2018, 9:16 a.m.
fix ticket: 7504

Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
---
 libavcodec/aacenc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Hendrik Leppkes Nov. 2, 2018, 10:13 a.m.
On Fri, Nov 2, 2018 at 10:17 AM Steven Liu <lq@chinaffmpeg.org> wrote:
>
> fix ticket: 7504
>
> Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
> ---
>  libavcodec/aacenc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libavcodec/aacenc.c b/libavcodec/aacenc.c
> index 4d0abb107f..26175bdb39 100644
> --- a/libavcodec/aacenc.c
> +++ b/libavcodec/aacenc.c
> @@ -973,7 +973,7 @@ static av_cold int aac_encode_init(AVCodecContext *avctx)
>      /* Channel map and unspecified bitrate guessing */
>      s->channels = avctx->channels;
>
> -    s->needs_pce = 1;
> +    s->needs_pce = s->options.pce;
>      for (i = 0; i < FF_ARRAY_ELEMS(aac_normal_chan_layouts); i++) {
>          if (avctx->channel_layout == aac_normal_chan_layouts[i]) {
>              s->needs_pce = s->options.pce;

This doesn't seem correct. PCE should be used if you feed the encode a
channel layout thats not supported without PCE - and the loop just
below your changed line will turn PCE off (or rather defer to the user
option) if a layout is used that doesn't need PCE.
If you just blindly turn PCE off, you'll cause  endless regressions
for uncommon channel layouts.

- Hendrik
Hendrik Leppkes Nov. 2, 2018, 10:14 a.m.
On Fri, Nov 2, 2018 at 11:13 AM Hendrik Leppkes <h.leppkes@gmail.com> wrote:
>
> On Fri, Nov 2, 2018 at 10:17 AM Steven Liu <lq@chinaffmpeg.org> wrote:
> >
> > fix ticket: 7504
> >
> > Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
> > ---
> >  libavcodec/aacenc.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/libavcodec/aacenc.c b/libavcodec/aacenc.c
> > index 4d0abb107f..26175bdb39 100644
> > --- a/libavcodec/aacenc.c
> > +++ b/libavcodec/aacenc.c
> > @@ -973,7 +973,7 @@ static av_cold int aac_encode_init(AVCodecContext *avctx)
> >      /* Channel map and unspecified bitrate guessing */
> >      s->channels = avctx->channels;
> >
> > -    s->needs_pce = 1;
> > +    s->needs_pce = s->options.pce;
> >      for (i = 0; i < FF_ARRAY_ELEMS(aac_normal_chan_layouts); i++) {
> >          if (avctx->channel_layout == aac_normal_chan_layouts[i]) {
> >              s->needs_pce = s->options.pce;
>
> This doesn't seem correct. PCE should be used if you feed the encode a
> channel layout thats not supported without PCE - and the loop just
> below your changed line will turn PCE off (or rather defer to the user
> option) if a layout is used that doesn't need PCE.
> If you just blindly turn PCE off, you'll cause  endless regressions
> for uncommon channel layouts.
>

What I forgot to say, the actual problem with HLS is that it doesn't
deal properly with PCEs. The solution is not to turn off PCEs, but to
actually fix the ADTS muxer to properly handle PCE.

- Hendrik
Carl Eugen Hoyos Nov. 2, 2018, 12:13 p.m.
2018-11-02 11:13 GMT+01:00, Hendrik Leppkes <h.leppkes@gmail.com>:
> On Fri, Nov 2, 2018 at 10:17 AM Steven Liu <lq@chinaffmpeg.org> wrote:
>>
>> fix ticket: 7504
>>
>> Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
>> ---
>>  libavcodec/aacenc.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libavcodec/aacenc.c b/libavcodec/aacenc.c
>> index 4d0abb107f..26175bdb39 100644
>> --- a/libavcodec/aacenc.c
>> +++ b/libavcodec/aacenc.c
>> @@ -973,7 +973,7 @@ static av_cold int aac_encode_init(AVCodecContext
>> *avctx)
>>      /* Channel map and unspecified bitrate guessing */
>>      s->channels = avctx->channels;
>>
>> -    s->needs_pce = 1;
>> +    s->needs_pce = s->options.pce;
>>      for (i = 0; i < FF_ARRAY_ELEMS(aac_normal_chan_layouts); i++) {
>>          if (avctx->channel_layout == aac_normal_chan_layouts[i]) {
>>              s->needs_pce = s->options.pce;
>
> This doesn't seem correct. PCE should be used if you feed the
> encode a channel layout thats not supported without PCE

Isn't the main issue that FFmpeg suddenly started to assume for
some very common layouts that they are not supported while
they were supported without PCE before and worked fine in every
sensible use-case?
(And that apparently typical decoders do not support these "new"
layouts with PCE?)

Carl Eugen
Hendrik Leppkes Nov. 2, 2018, 12:20 p.m.
On Fri, Nov 2, 2018 at 1:13 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>
> 2018-11-02 11:13 GMT+01:00, Hendrik Leppkes <h.leppkes@gmail.com>:
> > On Fri, Nov 2, 2018 at 10:17 AM Steven Liu <lq@chinaffmpeg.org> wrote:
> >>
> >> fix ticket: 7504
> >>
> >> Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
> >> ---
> >>  libavcodec/aacenc.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/libavcodec/aacenc.c b/libavcodec/aacenc.c
> >> index 4d0abb107f..26175bdb39 100644
> >> --- a/libavcodec/aacenc.c
> >> +++ b/libavcodec/aacenc.c
> >> @@ -973,7 +973,7 @@ static av_cold int aac_encode_init(AVCodecContext
> >> *avctx)
> >>      /* Channel map and unspecified bitrate guessing */
> >>      s->channels = avctx->channels;
> >>
> >> -    s->needs_pce = 1;
> >> +    s->needs_pce = s->options.pce;
> >>      for (i = 0; i < FF_ARRAY_ELEMS(aac_normal_chan_layouts); i++) {
> >>          if (avctx->channel_layout == aac_normal_chan_layouts[i]) {
> >>              s->needs_pce = s->options.pce;
> >
> > This doesn't seem correct. PCE should be used if you feed the
> > encode a channel layout thats not supported without PCE
>
> Isn't the main issue that FFmpeg suddenly started to assume for
> some very common layouts that they are not supported while
> they were supported without PCE before and worked fine in every
> sensible use-case?

How can a layout be supported without PCE if its not part of the
default channel layout map?
Does it give you a best guess match? That seems terrible. It should
explicitly encode what you give it, and not "interpet" it, thats up to
the user.

- Hendrik
Carl Eugen Hoyos Nov. 2, 2018, 12:23 p.m.
2018-11-02 13:20 GMT+01:00, Hendrik Leppkes <h.leppkes@gmail.com>:
> On Fri, Nov 2, 2018 at 1:13 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:

>> Isn't the main issue that FFmpeg suddenly started to assume for
>> some very common layouts that they are not supported while
>> they were supported without PCE before and worked fine in every
>> sensible use-case?
>
> How can a layout be supported without PCE if its not part of the
> default channel layout map?

IRL, there is no difference between 5.1 back and 5.1 side.

Carl Eugen

Patch hide | download patch | download mbox

diff --git a/libavcodec/aacenc.c b/libavcodec/aacenc.c
index 4d0abb107f..26175bdb39 100644
--- a/libavcodec/aacenc.c
+++ b/libavcodec/aacenc.c
@@ -973,7 +973,7 @@  static av_cold int aac_encode_init(AVCodecContext *avctx)
     /* Channel map and unspecified bitrate guessing */
     s->channels = avctx->channels;
 
-    s->needs_pce = 1;
+    s->needs_pce = s->options.pce;
     for (i = 0; i < FF_ARRAY_ELEMS(aac_normal_chan_layouts); i++) {
         if (avctx->channel_layout == aac_normal_chan_layouts[i]) {
             s->needs_pce = s->options.pce;