diff mbox series

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

Message ID 1642070709-3104-2-git-send-email-lance.lmwang@gmail.com
State New
Headers show
Series [FFmpeg-devel,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

Commit Message

Lance Wang Jan. 13, 2022, 10:45 a.m. UTC 
From: Limin Wang <lance.lmwang@gmail.com>

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 | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Comments

Martin Storsjö Jan. 13, 2022, 9:57 p.m. UTC | #1 
On Thu, 13 Jan 2022, lance.lmwang@gmail.com wrote:

> From: Limin Wang <lance.lmwang@gmail.com>
>
> 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 | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
> index a55bef8..995ee37 100644
> --- a/libavcodec/libopenh264enc.c
> +++ b/libavcodec/libopenh264enc.c
> @@ -220,26 +220,27 @@ 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:
> +        s->profile = PRO_HIGH;

I don't think we should reuse the s->profile field for this value here.

In practice, both FF_PROFILE_H264_HIGH and PRO_HIGH have the same values, 
but they're enums from different namespaces, so would it be clearer to use 
one variable for profiles set with FF_PROFILE_* and one with the PRO_* 
values?

>         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:
> +        s->profile = PRO_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:
> +        s->profile = PRO_BASELINE;
>         param.iEntropyCodingModeFlag = 0;
>         av_log(avctx, AV_LOG_VERBOSE, "Using CAVLC, "
>                "select EProfileIdc PRO_BASELINE in libopenh264.\n");
>         break;
>     default:
> +        s->profile = PRO_BASELINE;
>         param.iEntropyCodingModeFlag = 0;
>         av_log(avctx, AV_LOG_WARNING, "Unsupported profile, "
>                "select EProfileIdc PRO_BASELINE in libopenh264.\n");
> @@ -251,6 +252,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;

So this assignment is what was missing, and was what caused the incorrect 
conclusion in d3a7bdd4ac54349aea9150a234478635d50ebd87? I think it'd be 
good to explicitly spell this out in the commit message, saying that 
d3a7bdd4ac54349aea9150a234478635d50ebd87 was based on incorrect 
conclusions because we had missed to set uiProfileIdc.

// Martin
Lance Wang Jan. 14, 2022, 3:17 a.m. UTC | #2 
On Thu, Jan 13, 2022 at 11:57:36PM +0200, Martin Storsjö wrote:
> On Thu, 13 Jan 2022, lance.lmwang@gmail.com wrote:
> 
> > From: Limin Wang <lance.lmwang@gmail.com>
> > 
> > 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 | 8 +++++---
> > 1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
> > index a55bef8..995ee37 100644
> > --- a/libavcodec/libopenh264enc.c
> > +++ b/libavcodec/libopenh264enc.c
> > @@ -220,26 +220,27 @@ 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:
> > +        s->profile = PRO_HIGH;
> 
> I don't think we should reuse the s->profile field for this value here.
> 
> In practice, both FF_PROFILE_H264_HIGH and PRO_HIGH have the same values,
> but they're enums from different namespaces, so would it be clearer to use
> one variable for profiles set with FF_PROFILE_* and one with the PRO_*
> values?

Yes, I think they're same value by specs, I'll delete the assignment for PRO_* 
to make it be cleaner.

> 
> >         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:
> > +        s->profile = PRO_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:
> > +        s->profile = PRO_BASELINE;
> >         param.iEntropyCodingModeFlag = 0;
> >         av_log(avctx, AV_LOG_VERBOSE, "Using CAVLC, "
> >                "select EProfileIdc PRO_BASELINE in libopenh264.\n");
> >         break;
> >     default:
> > +        s->profile = PRO_BASELINE;
> >         param.iEntropyCodingModeFlag = 0;
> >         av_log(avctx, AV_LOG_WARNING, "Unsupported profile, "
> >                "select EProfileIdc PRO_BASELINE in libopenh264.\n");
> > @@ -251,6 +252,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;
> 
> So this assignment is what was missing, and was what caused the incorrect
> conclusion in d3a7bdd4ac54349aea9150a234478635d50ebd87? I think it'd be good
> to explicitly spell this out in the commit message, saying that

OK, will add the following message into commit message:
d3a7bdd4ac54349aea9150a234478635d50ebd87 was based on incorrect conclusions
because we had missed to set uiProfileIdc.

> 
> // Martin
>
diff mbox series

Patch

diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
index a55bef8..995ee37 100644
--- a/libavcodec/libopenh264enc.c
+++ b/libavcodec/libopenh264enc.c
@@ -220,26 +220,27 @@  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:
+        s->profile = PRO_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:
+        s->profile = PRO_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:
+        s->profile = PRO_BASELINE;
         param.iEntropyCodingModeFlag = 0;
         av_log(avctx, AV_LOG_VERBOSE, "Using CAVLC, "
                "select EProfileIdc PRO_BASELINE in libopenh264.\n");
         break;
     default:
+        s->profile = PRO_BASELINE;
         param.iEntropyCodingModeFlag = 0;
         av_log(avctx, AV_LOG_WARNING, "Unsupported profile, "
                "select EProfileIdc PRO_BASELINE in libopenh264.\n");
@@ -251,6 +252,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) {