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

Paul B Mahol onemda at gmail.com
Fri Dec 7 14:15:29 EET 2018


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.


More information about the ffmpeg-devel mailing list