| Message ID | 1511408256-24718-2-git-send-email-vdixit@akamai.com |
|---|---|
| State | Superseded |
| Headers | show |
2017-11-23 4:37 GMT+01:00 <vdixit@akamai.com>: > + s = x264_encoder_headers(x4->enc, &nal, &nnal); > + if (avctx->profile == FF_PROFILE_UNKNOWN) > + avctx->profile = nal->p_payload[5]; > + if (avctx->level == FF_LEVEL_UNKNOWN) > + avctx->level = nal->p_payload[7]; Why are these conditional? (Do we really guarantee that our profile and level fields contain the exact same values as the nal payload?) Carl Eugen
On 11/23/17, 4:21 PM, "Carl Eugen Hoyos" <ceffmpeg@gmail.com> wrote: >2017-11-23 4:37 GMT+01:00 <vdixit@akamai.com>: > >> + s = x264_encoder_headers(x4->enc, &nal, &nnal); >> + if (avctx->profile == FF_PROFILE_UNKNOWN) >> + avctx->profile = nal->p_payload[5]; >> + if (avctx->level == FF_LEVEL_UNKNOWN) >> + avctx->level = nal->p_payload[7]; > >Why are these conditional? We didn’t want to overwrite profile and level, if user had already set it. Comments in avcodec.h mentioned, ‘encoding: Set by user’, for profile and level. > >(Do we really guarantee that our profile and level >fields contain the exact same values as the nal >payload?) There is no guarantee. But instead of unknown profile and level, setting profile and level whenever possible could enhance feature-set of certain muxers, like hlsenc. > >Carl Eugen regards, Karthick
2017-11-23 12:47 GMT+01:00 Jeyapal, Karthick <kjeyapal@akamai.com>: > On 11/23/17, 4:21 PM, "Carl Eugen Hoyos" <ceffmpeg@gmail.com> wrote: > >>2017-11-23 4:37 GMT+01:00 <vdixit@akamai.com>: >> >>> + s = x264_encoder_headers(x4->enc, &nal, &nnal); >>> + if (avctx->profile == FF_PROFILE_UNKNOWN) >>> + avctx->profile = nal->p_payload[5]; >>> + if (avctx->level == FF_LEVEL_UNKNOWN) >>> + avctx->level = nal->p_payload[7]; >> >>Why are these conditional? > We didn’t want to overwrite profile and level, if user had already set it. So if x264 changes these values because of contradicting user requests, we write the wrong values into the hls header? Carl Eugen
On 11/23/17, 5:33 PM, "Carl Eugen Hoyos" <ceffmpeg@gmail.com> wrote: >2017-11-23 12:47 GMT+01:00 Jeyapal, Karthick <kjeyapal@akamai.com>: >> On 11/23/17, 4:21 PM, "Carl Eugen Hoyos" <ceffmpeg@gmail.com> wrote: >> >>>2017-11-23 4:37 GMT+01:00 <vdixit@akamai.com>: >> >>>> + s = x264_encoder_headers(x4->enc, &nal, &nnal); >>>> + if (avctx->profile == FF_PROFILE_UNKNOWN) >>>> + avctx->profile = nal->p_payload[5]; >>>> + if (avctx->level == FF_LEVEL_UNKNOWN) >>>> + avctx->level = nal->p_payload[7]; >>> >>>Why are these conditional? >> We didn’t want to overwrite profile and level, if user had already set it. > >So if x264 changes these values because of contradicting user >requests, we write the wrong values into the hls header? Yes, that is true. We were afraid to set profile and level unconditionally, because we thought it could get rejected by maintainers(due to that comment in avcodec.h). Now, if you suggest us to remove those conditions, we would be happy to do it. Please advise. Thanks and regards, Karthick
2017-11-23 13:56 GMT+01:00 Jeyapal, Karthick <kjeyapal@akamai.com>: > > On 11/23/17, 5:33 PM, "Carl Eugen Hoyos" <ceffmpeg@gmail.com> wrote: > >>2017-11-23 12:47 GMT+01:00 Jeyapal, Karthick <kjeyapal@akamai.com>: >>> On 11/23/17, 4:21 PM, "Carl Eugen Hoyos" <ceffmpeg@gmail.com> wrote: >>> >>>>2017-11-23 4:37 GMT+01:00 <vdixit@akamai.com>: >>> >>>>> + s = x264_encoder_headers(x4->enc, &nal, &nnal); >>>>> + if (avctx->profile == FF_PROFILE_UNKNOWN) >>>>> + avctx->profile = nal->p_payload[5]; >>>>> + if (avctx->level == FF_LEVEL_UNKNOWN) >>>>> + avctx->level = nal->p_payload[7]; >>>> >>>>Why are these conditional? >>> We didn’t want to overwrite profile and level, if user had already set it. >> >>So if x264 changes these values because of contradicting user >>requests, we write the wrong values into the hls header? > > Yes, that is true. > We were afraid to set profile and level unconditionally, because we > thought it could get rejected by maintainers Very smart, you have learned quickly! (I wish that wouldn't be the message we send to new contributors...) > (due to that comment in avcodec.h). I sent a patch. > Now, if you suggest us to remove those conditions, we would be > happy to do it. I believe other encoders already overwrite it and I believe in this case it would be a particularly bad idea not to overwrite it. Carl Eugen
On 11/23/17, 8:34 PM, "Carl Eugen Hoyos" <ceffmpeg@gmail.com> wrote: >2017-11-23 13:56 GMT+01:00 Jeyapal, Karthick <kjeyapal@akamai.com>: >> >> On 11/23/17, 5:33 PM, "Carl Eugen Hoyos" <ceffmpeg@gmail.com> wrote: >> >>>2017-11-23 12:47 GMT+01:00 Jeyapal, Karthick <kjeyapal@akamai.com>: >>>> On 11/23/17, 4:21 PM, "Carl Eugen Hoyos" <ceffmpeg@gmail.com> wrote: >>>> >>>>>2017-11-23 4:37 GMT+01:00 <vdixit@akamai.com>: >>>> >>>>>> + s = x264_encoder_headers(x4->enc, &nal, &nnal); >>>>>> + if (avctx->profile == FF_PROFILE_UNKNOWN) >>>>>> + avctx->profile = nal->p_payload[5]; >>>>>> + if (avctx->level == FF_LEVEL_UNKNOWN) >>>>>> + avctx->level = nal->p_payload[7]; >>>>> >>>>>Why are these conditional? >>>> We didn’t want to overwrite profile and level, if user had already set it. >>> >>>So if x264 changes these values because of contradicting user >>>requests, we write the wrong values into the hls header? >> >> Yes, that is true. > >> We were afraid to set profile and level unconditionally, because we >> thought it could get rejected by maintainers > >Very smart, you have learned quickly! Oh yeah :) Quite a learning experience working with you folks! >(I wish that wouldn't be the message we send to new contributors...) > >> (due to that comment in avcodec.h). > >I sent a patch. Thanks. > >> Now, if you suggest us to remove those conditions, we would be >> happy to do it. > >I believe other encoders already overwrite it and I believe in this >case it would be a particularly bad idea not to overwrite it. Sure. Will do it. Regards, Karthick
diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c index 9c67c91..eeafe31 100644 --- a/libavcodec/libx264.c +++ b/libavcodec/libx264.c @@ -454,6 +454,9 @@ static av_cold int X264_init(AVCodecContext *avctx) X264Context *x4 = avctx->priv_data; AVCPBProperties *cpb_props; int sw,sh; + x264_nal_t *nal; + uint8_t *p; + int nnal, s, i; if (avctx->global_quality > 0) av_log(avctx, AV_LOG_WARNING, "-qscale is ignored, -crf is recommended.\n"); @@ -799,12 +802,13 @@ FF_ENABLE_DEPRECATION_WARNINGS if (!x4->enc) return AVERROR_EXTERNAL; - if (avctx->flags & AV_CODEC_FLAG_GLOBAL_HEADER) { - x264_nal_t *nal; - uint8_t *p; - int nnal, s, i; + s = x264_encoder_headers(x4->enc, &nal, &nnal); + if (avctx->profile == FF_PROFILE_UNKNOWN) + avctx->profile = nal->p_payload[5]; + if (avctx->level == FF_LEVEL_UNKNOWN) + avctx->level = nal->p_payload[7]; - s = x264_encoder_headers(x4->enc, &nal, &nnal); + if (avctx->flags & AV_CODEC_FLAG_GLOBAL_HEADER) { avctx->extradata = p = av_mallocz(s + AV_INPUT_BUFFER_PADDING_SIZE); if (!p) return AVERROR(ENOMEM);