[FFmpeg-devel] [PATCH] omx: Add support for specifying H.264 profile [v3]

[Mark Thompson]

On 09/02/17 20:54, Mark Thompson wrote:
> On 08/02/17 19:21, Takayuki 'January June' Suwa wrote:
>> From: Takayuki 'January June' Suwa <jjsuwa at users.noreply.github.com>
>>
>> This adds "-profile[:v] profile_name"-style option IOW.
>>
>> Now default/unknown profile means FF_PROFILE_H264_HIGH strictly, for both better coding style and preserving the original behavior.
>> ---
>>  libavcodec/omx.c | 20 ++++++++++++++++++++
>>  1 file changed, 20 insertions(+)
>>
>> diff --git a/libavcodec/omx.c b/libavcodec/omx.c
>> index 16df50e..6ce71e9 100644
>> --- a/libavcodec/omx.c
>> +++ b/libavcodec/omx.c
>> @@ -226,6 +226,7 @@ typedef struct OMXCodecContext {
>>      int output_buf_size;
>>  
>>      int input_zerocopy;
>> +    int profile;
>>  } OMXCodecContext;
>>  
>>  static void append_buffer(pthread_mutex_t *mutex, pthread_cond_t *cond,
>> @@ -523,6 +524,21 @@ static av_cold int omx_component_init(AVCodecContext *avctx, const char *role)
>>          CHECK(err);
>>          avc.nBFrames = 0;
>>          avc.nPFrames = avctx->gop_size - 1;
>> +        switch (s->profile) {
>> +        case FF_PROFILE_H264_BASELINE:
>> +            avctx->profile = s->profile;
> 
> AVCodecContext.profile is a user-set input field of AVCodecContext for encoders - you should only be reading it here, not writing to it (see <http://git.videolan.org/?p=ffmpeg.git;a=blob;f=libavcodec/avcodec.h;h=1e681e989bef52d56717af78705c78f4b170b30c;hb=HEAD#l3188>).
> 
> I think the right thing to do here is to follow libx264 and do:
> 
> if (s->profile is set) {
>     use s->profile;
> } else if (avctx->profile is set) {
>     use avctx->profile;
> } else {
>     use the default profile (i.e. high);
> }
> 
> See <http://git.videolan.org/?p=ffmpeg.git;a=blob;f=libavcodec/libx264.c;h=b11ede61988639ea82f7f8f378ef45792fda779d;hb=HEAD#l676> (the default is hidden inside libx264 by just not specifying the profile at all).  While this ends up being equivalent for the ffmpeg utility, it makes it easier and more consistent for lavc users because they can use the common option to all encoders rather than having to set a private option for this encoder.

To clarify, since that was a bit unclear: if the user hasn't supplied any particular profile then the default should be to not set the profile parameter at all (i.e. let the OpenMAX implementation choose).

>> +            avc.eProfile = OMX_VIDEO_AVCProfileBaseline;
>> +            break;
>> +        case FF_PROFILE_H264_MAIN:
>> +            avctx->profile = s->profile;
>> +            avc.eProfile = OMX_VIDEO_AVCProfileMain;
>> +            break;
>> +        case FF_PROFILE_H264_HIGH:
>> +        default:
>> +            avctx->profile = s->profile;
>> +            avc.eProfile = OMX_VIDEO_AVCProfileHigh;
>> +            break;
>> +        }
>>          err = OMX_SetParameter(s->handle, OMX_IndexParamVideoAvc, &avc);
>>          CHECK(err);
>>      }
>> @@ -884,6 +900,10 @@ static const AVOption options[] = {
>>      { "omx_libname", "OpenMAX library name", OFFSET(libname), AV_OPT_TYPE_STRING, { 0 }, 0, 0, VDE },
>>      { "omx_libprefix", "OpenMAX library prefix", OFFSET(libprefix), AV_OPT_TYPE_STRING, { 0 }, 0, 0, VDE },
>>      { "zerocopy", "Try to avoid copying input frames if possible", OFFSET(input_zerocopy), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, 1, VE },
>> +    { "profile",  "Set the encoding profile", OFFSET(profile), AV_OPT_TYPE_INT,   { .i64 = 0 },                        0, FF_PROFILE_H264_HIGH, VE, "profile" },
>> +    { "baseline", "",                         0,               AV_OPT_TYPE_CONST, { .i64 = FF_PROFILE_H264_BASELINE }, 0, 0, VE, "profile" },
>> +    { "main",     "",                         0,               AV_OPT_TYPE_CONST, { .i64 = FF_PROFILE_H264_MAIN },     0, 0, VE, "profile" },
>> +    { "high",     "",                         0,               AV_OPT_TYPE_CONST, { .i64 = FF_PROFILE_H264_HIGH },     0, 0, VE, "profile" },
>>      { NULL }
>>  };
> 
> The options look good to me now.  (Slightly disappointed that it's baseline rather than constrained baseline, but I can see that that's an OpenMAX problem which we can't solve here.)
> 
> Thanks,
> 
> - Mark
>