| Message ID | 20181102091634.66159-1-lq@chinaffmpeg.org |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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;
fix ticket: 7504 Signed-off-by: Steven Liu <lq@chinaffmpeg.org> --- libavcodec/aacenc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)