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

[Michael Niedermayer]

On Fri, Dec 07, 2018 at 01:43:37PM +0100, Hendrik Leppkes wrote:
> 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.

codec_tag (from the demuxer) is also needed by many other decoders
some of these can be seen with this:
git grep 'codec_tag *==' libavcodec/


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Everything should be made as simple as possible, but not simpler.
-- Albert Einstein
-------------- 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/cfd2de68/attachment.sig>