diff mbox series

[FFmpeg-devel,v2,2/3] avcodec/libopenh264enc: make the profile configuablable correctly

Message ID 1642150837-22960-2-git-send-email-lance.lmwang@gmail.com
State New
Headers show
Series [FFmpeg-devel,v2,1/3] avcodec/libopenh264enc: support for colorspace and range information | expand

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/make_ppc success Make finished
andriy/make_fate_ppc success Make fate finished
andriy/make_aarch64_jetson success Make finished
andriy/make_fate_aarch64_jetson success Make fate finished
andriy/makeaarch64_jetson warning New warnings during build

Commit Message

Lance Wang Jan. 14, 2022, 9 a.m. UTC 
From: Limin Wang <lance.lmwang@gmail.com>

d3a7bdd4ac54349aea9150a234478635d50ebd87 was based on incorrect conclusions
because we had missed to set uiProfileIdc.

If the version of libopenh264 >= 1.8, we can't configured main profile as
expected, below is the testing cli:

ffmpeg -y -f lavfi -i testsrc -c:v libopenh264 -profile:v main -frames:v 1 test.ts
It'll report:
[libopenh264 @ 0x5638300] Unsupported profile, select EProfileIdc PRO_BASELINE in libopenh264.

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 libavcodec/libopenh264enc.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Comments

Martin Storsjö Jan. 14, 2022, 9:28 a.m. UTC | #1 
On Fri, 14 Jan 2022, lance.lmwang@gmail.com wrote:

> From: Limin Wang <lance.lmwang@gmail.com>
>
> d3a7bdd4ac54349aea9150a234478635d50ebd87 was based on incorrect conclusions
> because we had missed to set uiProfileIdc.
>
> If the version of libopenh264 >= 1.8, we can't configured main profile as
> expected, below is the testing cli:
>
> ffmpeg -y -f lavfi -i testsrc -c:v libopenh264 -profile:v main -frames:v 1 test.ts
> It'll report:
> [libopenh264 @ 0x5638300] Unsupported profile, select EProfileIdc PRO_BASELINE in libopenh264.

Given the referenced commit, the fact that you can't use main profile with 
1.8 (or high profile with older) isn't a surprise as that was part of the 
deliberate design in that commit, so I'm not sure if this 
is a good example to include. Or reword it to say that "due to the 
limitations set in <commit>, you weren't able to use main profile with 
OpenH264 1.8, or high profile with older versions".

> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
> libavcodec/libopenh264enc.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
> index 5b5914c..8e27edb 100644
> --- a/libavcodec/libopenh264enc.c
> +++ b/libavcodec/libopenh264enc.c
> @@ -220,19 +220,16 @@ static av_cold int svc_encode_init(AVCodecContext *avctx)
> #endif
>
>     switch (s->profile) {
> -#if OPENH264_VER_AT_LEAST(1, 8)
>     case FF_PROFILE_H264_HIGH:
>         param.iEntropyCodingModeFlag = 1;
>         av_log(avctx, AV_LOG_VERBOSE, "Using CABAC, "
>                 "select EProfileIdc PRO_HIGH in libopenh264.\n");
>         break;
> -#else
>     case FF_PROFILE_H264_MAIN:
>         param.iEntropyCodingModeFlag = 1;
>         av_log(avctx, AV_LOG_VERBOSE, "Using CABAC, "
>                 "select EProfileIdc PRO_MAIN in libopenh264.\n");
>         break;
> -#endif
>     case FF_PROFILE_H264_CONSTRAINED_BASELINE:
>     case FF_PROFILE_UNKNOWN:
>         param.iEntropyCodingModeFlag = 0;

For this fallback case, I think we should set s->profile to 
FF_PROFILE_H264_CONSTRAINED_BASELINE or something similar (does OpenH264 
work correctly with FF_PROFILE_H264_CONSTRAINED_BASELINE too, or only with 
FF_PROFILE_H264_BASELINE which is equal to PRO_BASELINE), as we otherwise 
would be setting unknown values into uiProfileIdc.

// Martin
Lance Wang Jan. 14, 2022, 9:43 a.m. UTC | #2 
On Fri, Jan 14, 2022 at 11:28:14AM +0200, Martin Storsjö wrote:
> On Fri, 14 Jan 2022, lance.lmwang@gmail.com wrote:
> 
> > From: Limin Wang <lance.lmwang@gmail.com>
> > 
> > d3a7bdd4ac54349aea9150a234478635d50ebd87 was based on incorrect conclusions
> > because we had missed to set uiProfileIdc.
> > 
> > If the version of libopenh264 >= 1.8, we can't configured main profile as
> > expected, below is the testing cli:
> > 
> > ffmpeg -y -f lavfi -i testsrc -c:v libopenh264 -profile:v main -frames:v 1 test.ts
> > It'll report:
> > [libopenh264 @ 0x5638300] Unsupported profile, select EProfileIdc PRO_BASELINE in libopenh264.
> 
> Given the referenced commit, the fact that you can't use main profile with
> 1.8 (or high profile with older) isn't a surprise as that was part of the
> deliberate design in that commit, so I'm not sure if this is a good example
> to include. Or reword it to say that "due to the limitations set in
> <commit>, you weren't able to use main profile with OpenH264 1.8, or high
> profile with older versions".

Sure, will reword the message as suggestion.

> 
> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > ---
> > libavcodec/libopenh264enc.c | 4 +---
> > 1 file changed, 1 insertion(+), 3 deletions(-)
> > 
> > diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
> > index 5b5914c..8e27edb 100644
> > --- a/libavcodec/libopenh264enc.c
> > +++ b/libavcodec/libopenh264enc.c
> > @@ -220,19 +220,16 @@ static av_cold int svc_encode_init(AVCodecContext *avctx)
> > #endif
> > 
> >     switch (s->profile) {
> > -#if OPENH264_VER_AT_LEAST(1, 8)
> >     case FF_PROFILE_H264_HIGH:
> >         param.iEntropyCodingModeFlag = 1;
> >         av_log(avctx, AV_LOG_VERBOSE, "Using CABAC, "
> >                 "select EProfileIdc PRO_HIGH in libopenh264.\n");
> >         break;
> > -#else
> >     case FF_PROFILE_H264_MAIN:
> >         param.iEntropyCodingModeFlag = 1;
> >         av_log(avctx, AV_LOG_VERBOSE, "Using CABAC, "
> >                 "select EProfileIdc PRO_MAIN in libopenh264.\n");
> >         break;
> > -#endif
> >     case FF_PROFILE_H264_CONSTRAINED_BASELINE:
> >     case FF_PROFILE_UNKNOWN:
> >         param.iEntropyCodingModeFlag = 0;
> 
> For this fallback case, I think we should set s->profile to
> FF_PROFILE_H264_CONSTRAINED_BASELINE or something similar (does OpenH264
> work correctly with FF_PROFILE_H264_CONSTRAINED_BASELINE too, or only with
> FF_PROFILE_H264_BASELINE which is equal to PRO_BASELINE), as we otherwise
> would be setting unknown values into uiProfileIdc.

Sure, will update to set profile to FF_PROFILE_H264_CONSTRAINED_BASELINE for unknown case.

> 
> // Martin
>
diff mbox series

Patch

diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
index 5b5914c..8e27edb 100644
--- a/libavcodec/libopenh264enc.c
+++ b/libavcodec/libopenh264enc.c
@@ -220,19 +220,16 @@  static av_cold int svc_encode_init(AVCodecContext *avctx)
 #endif
 
     switch (s->profile) {
-#if OPENH264_VER_AT_LEAST(1, 8)
     case FF_PROFILE_H264_HIGH:
         param.iEntropyCodingModeFlag = 1;
         av_log(avctx, AV_LOG_VERBOSE, "Using CABAC, "
                 "select EProfileIdc PRO_HIGH in libopenh264.\n");
         break;
-#else
     case FF_PROFILE_H264_MAIN:
         param.iEntropyCodingModeFlag = 1;
         av_log(avctx, AV_LOG_VERBOSE, "Using CABAC, "
                 "select EProfileIdc PRO_MAIN in libopenh264.\n");
         break;
-#endif
     case FF_PROFILE_H264_CONSTRAINED_BASELINE:
     case FF_PROFILE_UNKNOWN:
         param.iEntropyCodingModeFlag = 0;
@@ -251,6 +248,7 @@  static av_cold int svc_encode_init(AVCodecContext *avctx)
     param.sSpatialLayers[0].fFrameRate          = param.fMaxFrameRate;
     param.sSpatialLayers[0].iSpatialBitrate     = param.iTargetBitrate;
     param.sSpatialLayers[0].iMaxSpatialBitrate  = param.iMaxBitrate;
+    param.sSpatialLayers[0].uiProfileIdc        = s->profile;
 
 #if OPENH264_VER_AT_LEAST(1, 7)
     if (avctx->sample_aspect_ratio.num && avctx->sample_aspect_ratio.den) {