diff mbox

[FFmpeg-devel,v2,2/3] avcodec/libx264:setting profile and level in avcodec context

Message ID 1511516249-20195-1-git-send-email-kjeyapal@akamai.com
State Superseded
Headers show

Commit Message

Jeyapal, Karthick Nov. 24, 2017, 9:37 a.m. UTC 
From: Vishwanath Dixit <vdixit@akamai.com>

---
 libavcodec/libx264.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

Comments

Mark Thompson Nov. 24, 2017, 10:55 a.m. UTC | #1 
On 24/11/17 09:37, Karthick J wrote:
> From: Vishwanath Dixit <vdixit@akamai.com>
> 
> ---
>  libavcodec/libx264.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
> index 9c67c91..6b93aa8 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,11 @@ 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);
> +    avctx->profile = nal->p_payload[5];

AVCodecContext.profile should include some of the constraint_set_flags - see <http://git.videolan.org/?p=ffmpeg.git;a=blob;f=libavcodec/avcodec.h#l2842>.

> +    avctx->level = nal->p_payload[7];

I don't much like the hard-coding of the offsets here.  Maybe add some asserts so that it fails very quickly if something ever changes?  (I don't think it will with libx264, but if it does then this is going to be putting nonsense in the metadata.)

>  
> -        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);
> 

I think I preferred the version which only wrote the value if it isn't already set.  If the user specifies a profile then it should use that or fail.

- Mark
Carl Eugen Hoyos Nov. 24, 2017, 11:04 a.m. UTC | #2 
2017-11-24 11:55 GMT+01:00 Mark Thompson <sw@jkqxz.net>:
> On 24/11/17 09:37, Karthick J wrote:

>> -        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);
>>
>
> I think I preferred the version which only wrote the value
> if it isn't already set.  If the user specifies a profile then
> it should use that or fail.

The only way to let the user know that another profile was
used (and to fail) is to set the value unconditionally here.

Carl Eugen
Jeyapal, Karthick Nov. 24, 2017, 3:35 p.m. UTC | #3 
Thanks for your comments. I have uploaded new patchset v4 with suggested corrections.
Please ignore patchset v3.

On 11/24/17, 4:26 PM, "Mark Thompson" <sw@jkqxz.net> wrote:
[…]
>> +    s = x264_encoder_headers(x4->enc, &nal, &nnal);

>> +    avctx->profile = nal->p_payload[5];

>

>AVCodecContext.profile should include some of the constraint_set_flags - see <http://git.videolan.org/?p=ffmpeg.git;a=blob;f=libavcodec/avcodec.h#l2842>.

Great! I was not aware of this. This would also resolve my constraint_set_flags problem in hlsenc.
>

>> +    avctx->level = nal->p_payload[7];

>

>I don't much like the hard-coding of the offsets here.  Maybe add some asserts so that it fails very quickly if something ever changes?  (I don't think it will with libx264, but if it does then this is going to be putting nonsense in the metadata.)

Have added asserts to check start code and nal type.
>

>>  

>> -        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);

>> 

>

>I think I preferred the version which only wrote the value if it isn't already set.  If the user specifies a profile then it should use that or fail.

I am ok with both approaches. Let us take a final decision on this based on the result of the other patch submitted by carl.
http://ffmpeg.org/pipermail/ffmpeg-devel/2017-November/220701.html

regards,
Karthick
Dixit, Vishwanath Dec. 14, 2017, 11:32 a.m. UTC | #4 
>On 11/24/17, 9:06 PM, "Jeyapal, Karthick" <kjeyapal@akamai.com> wrote:

>

>Thanks for your comments. I have uploaded new patchset v4 with suggested corrections.

>Please ignore patchset v3.

>

>On 11/24/17, 4:26 PM, "Mark Thompson" <sw@jkqxz.net> wrote:

>[…]

>>> +    s = x264_encoder_headers(x4->enc, &nal, &nnal);

>>> +    avctx->profile = nal->p_payload[5];

>>

>>AVCodecContext.profile should include some of the constraint_set_flags - see <http://git.videolan.org/?p=ffmpeg.git;a=blob;f=libavcodec/avcodec.h#l2842>.

>Great! I was not aware of this. This would also resolve my constraint_set_flags problem in hlsenc.

>>

>>> +    avctx->level = nal->p_payload[7];

>>

>>I don't much like the hard-coding of the offsets here.  Maybe add some asserts so that it fails very quickly if something ever changes?  (I don't think it will with libx264, but if it does then this is going to be putting nonsense in the metadata.)

>Have added asserts to check start code and nal type.

>>

>>>  

>>> -        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);

>>> 

>>

>>I think I preferred the version which only wrote the value if it isn't already set.  If the user specifies a profile then it should use that or fail.

>I am ok with both approaches. Let us take a final decision on this based on the result of the other patch submitted by carl.

>http://ffmpeg.org/pipermail/ffmpeg-devel/2017-November/220701.html

>

>regards,

>Karthick


I have submitted patch set version v6 https://patchwork.ffmpeg.org/patch/6771/ which writes profile and level only if it is not already set. Earlier Mark Thompson had objected to setting profile and level unconditionally, and Carl sent a patch in avcodec.h to change the API definition in order to set profile and level by encoders. http://ffmpeg.org/pipermail/ffmpeg-devel/2017-November/220701.html. But that patch has not been merged yet and that discussion has gone cold. In the interest of some resolution for this patchset, we propose that this patch(v6) which writes profile and level only if is not set, as the patch with least objections. In this way, the current API behavior is not changed. Could this patchset atleast be merged? It doesn’t make sense to hold off merging the entire feature, till the API definition is changed.

Regards,
Vishwanath
diff mbox

Patch

diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
index 9c67c91..6b93aa8 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,11 @@  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);
+    avctx->profile = nal->p_payload[5];
+    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);