[FFmpeg-devel] [PATCH 1/4] avcodec/proresdec2: change profile only if it is unknown

[Hendrik Leppkes]

On Fri, Dec 7, 2018 at 1:15 PM Paul B Mahol <onemda at gmail.com> wrote:
>
> On 12/7/18, Michael Niedermayer <michael at niedermayer.cc> wrote:
> > On Fri, Dec 07, 2018 at 11:21:57AM +0100, Paul B Mahol wrote:
> >> On 12/7/18, Michael Niedermayer <michael at niedermayer.cc> wrote:
> >> > On Fri, Dec 07, 2018 at 10:28:09AM +0100, Paul B Mahol wrote:
> >> >> On 12/7/18, Michael Niedermayer <michael at niedermayer.cc> wrote:
> >> >> > On Wed, Dec 05, 2018 at 09:22:48PM +0100, Paul B Mahol wrote:
> >> >> >> Signed-off-by: Paul B Mahol <onemda at gmail.com>
> >> >> >> ---
> >> >> >>  libavcodec/proresdec2.c | 51
> >> >> >> ++++++++++++++++++++++-------------------
> >> >> >>  1 file changed, 27 insertions(+), 24 deletions(-)
> >> >> >>
> >> >> >> diff --git a/libavcodec/proresdec2.c b/libavcodec/proresdec2.c
> >> >> >> index 8581d797fb..80a76bbba1 100644
> >> >> >> --- a/libavcodec/proresdec2.c
> >> >> >> +++ b/libavcodec/proresdec2.c
> >> >> >> @@ -140,32 +140,35 @@ static av_cold int decode_init(AVCodecContext
> >> >> >> *avctx)
> >> >> >>@@ -140,6 +140,7 @@ static av_cold int decode_init(AVCodecContext
> >> >> >> *avctx)
> >> >> >>
> >> >> >>     avctx->bits_per_raw_sample = 10;
> >> >> >>
> >> >> >>+    if (avctx->profile == FF_PROFILE_UNKNOWN) {
> >> >> >>         switch (avctx->codec_tag) {
> >> >> >>         case MKTAG('a','p','c','o'):
> >> >> >>             avctx->profile = FF_PROFILE_PRORES_PROXY;
> >> >> >>@@ -155,16 +156,18 @@ static av_cold int decode_init(AVCodecContext
> >> >> >> *avctx)
> >> >> >>             break;
> >> >> >>         case MKTAG('a','p','4','h'):
> >> >> >>             avctx->profile = FF_PROFILE_PRORES_4444;
> >> >> >>-        avctx->bits_per_raw_sample = 12;
> >> >> >>             break;
> >> >> >>         case MKTAG('a','p','4','x'):
> >> >> >>             avctx->profile = FF_PROFILE_PRORES_XQ;
> >> >> >>-        avctx->bits_per_raw_sample = 12;
> >> >> >>             break;
> >> >> >>         default:
> >> >> >>-        avctx->profile = FF_PROFILE_UNKNOWN;
> >> >> >>             av_log(avctx, AV_LOG_WARNING, "Unknown prores profile
> >> >> >> %d\n",
> >> >> >> avctx->codec_tag);
> >> >> >>         }
> >> >> >>+    }
> >> >> >>+
> >> >> >>+    if (avctx->profile == FF_PROFILE_PRORES_XQ ||
> >> >> >>+        avctx->profile == FF_PROFILE_PRORES_4444)
> >> >> >>+        avctx->bits_per_raw_sample = 12;
> >> >> >
> >> >> > with this it would be possible to have 12bit output while the
> >> >> > profile
> >> >> > is set to one having 10bits and vice versa ?
> >> >>
> >> >> No, and neither with previous code.
> >> >>
> >> >> > maybe the profile should only be left if it is compatible with the
> >> >> > decoder output
> >> >>
> >> >> I do not follow.
> >> >
> >> > I may have misunderstood the intend of the patch
> >> > please document what this fixes in the commit message.
> >> >
> >> > AVCodecContext.profile is for decoding set by the decoder.
> >> > (thats how its documented in avcodec.h)
> >> >
> >> > So the obvious thought that this is about not overriding a profile
> >> > set by the user(app) or demuxer might have been flawed on my side
> >> > as that seems not possible in the API as it is documented
> >>
> >> You missed fact that profile is set via codecpar (which is than copied
> >> to codec context), never in codec context directly.
> >>
> >> Once again, what you want to change?
> >
> > As i said, please document in the commit message what this fixes.
> >
> > About codecpar, The documentation of the codec context did not allow
> > code outside the decoder to set profile and it now is set from outside
> > the decoder. That broadening of the interpretation is at least a source
> > for potential bugs.
> >
> > So, if i guess correctly, the issue this is about is that
> > codecpar has a profile set and that is given to
> > the decoder which then previously did override it and after the patch does
> > sometimes not.
> > So my original concern was that the value set in codecpar can be incorrect,
> > this value could come from the user application, demuxer or other source.
> >
> > As a specific example, the profile might be set to FF_PROFILE_PRORES_PROXY
> > That IIUC corresponds to "Apple ProRes 422 Proxy" in apples documents
> > and now the decoder could output 12bit 444 without correcting the profile.
> > IIUC this would be inconsistent
> >
> > This is not a major issue, its just metadata thats contradictionary
> >
> > Another minor issue is that this behavior is undocumented, or incorrectly
> > documented
> >
> > For example for width and height we document in avcodec.h:
> >      * - decoding: May be set by the user before opening the decoder if
> > known e.g.
> >      *             from the container. Some decoders will require the
> > dimensions
> >      *             to be set by the caller. During decoding, the decoder
> > may
> >      *             overwrite those values as required while parsing the
> > data.
> >      */
> >     int width, height;
> >
> > That says clearly that the user can set them and that they will be overriden
> > but
> > with profile we have:
> >
> >      * - decoding: Set by libavcodec.
> >      */
> >      int profile;
> >
> > Before this patch this was correct for prores, after the patch this
> > API documentation is at least misleading or incomplete
> > The decoder not just sometimes leaves the field but it sometimes also reads
> > the
> > field and uses it for the bits_per_raw_sample setting.
> >
> > What i want is to keep this all consistent and have documentation match
> > implementation. And things being documented well enough to use them based
> > on just the documentation
>
> prores decoder sets profile depending on codec_tag, which too might be
> incorrect.
> Do you propose to set codec_tag instead of profile from demuxer level? This way
> prores decoder code would not change.

It would be good to have one consistent way to inform the prores
decoder of the type of the bitstream. And codec_tag is already being
used for that today when demuxing from mov.

- Hendrik