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

[Michael Niedermayer]

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

Thanks

     
[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In a rich man's house there is no place to spit but his face.
-- Diogenes of Sinope
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20181207/d6140838/attachment.sig>