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

Submitted by Paul B Mahol on Dec. 5, 2018, 8:22 p.m.

Details

Message ID 20181205202248.16504-1-onemda@gmail.com
State New
Headers show

Commit Message

Paul B Mahol Dec. 5, 2018, 8:22 p.m.
Signed-off-by: Paul B Mahol <onemda@gmail.com>
---
 libavcodec/proresdec2.c | 51 ++++++++++++++++++++++-------------------
 1 file changed, 27 insertions(+), 24 deletions(-)

Comments

Carl Eugen Hoyos Dec. 5, 2018, 9:35 p.m.
2018-12-05 21:22 GMT+01:00, Paul B Mahol <onemda@gmail.com>:
> Signed-off-by: Paul B Mahol <onemda@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)
>
>      avctx->bits_per_raw_sample = 10;
>
> -    switch (avctx->codec_tag) {

I beg you, please don't reindent.

Thank you, Carl Eugen
Michael Niedermayer Dec. 7, 2018, 9:19 a.m.
On Wed, Dec 05, 2018 at 09:22:48PM +0100, Paul B Mahol wrote:
> Signed-off-by: Paul B Mahol <onemda@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 ?

maybe the profile should only be left if it is compatible with the
decoder output
Paul B Mahol Dec. 7, 2018, 9:28 a.m.
On 12/7/18, Michael Niedermayer <michael@niedermayer.cc> wrote:
> On Wed, Dec 05, 2018 at 09:22:48PM +0100, Paul B Mahol wrote:
>> Signed-off-by: Paul B Mahol <onemda@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.
Michael Niedermayer Dec. 7, 2018, 10 a.m.
On Fri, Dec 07, 2018 at 10:28:09AM +0100, Paul B Mahol wrote:
> On 12/7/18, Michael Niedermayer <michael@niedermayer.cc> wrote:
> > On Wed, Dec 05, 2018 at 09:22:48PM +0100, Paul B Mahol wrote:
> >> Signed-off-by: Paul B Mahol <onemda@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

thx
Paul B Mahol Dec. 7, 2018, 10:21 a.m.
On 12/7/18, Michael Niedermayer <michael@niedermayer.cc> wrote:
> On Fri, Dec 07, 2018 at 10:28:09AM +0100, Paul B Mahol wrote:
>> On 12/7/18, Michael Niedermayer <michael@niedermayer.cc> wrote:
>> > On Wed, Dec 05, 2018 at 09:22:48PM +0100, Paul B Mahol wrote:
>> >> Signed-off-by: Paul B Mahol <onemda@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?
Michael Niedermayer Dec. 7, 2018, 12:09 p.m.
On Fri, Dec 07, 2018 at 11:21:57AM +0100, Paul B Mahol wrote:
> On 12/7/18, Michael Niedermayer <michael@niedermayer.cc> wrote:
> > On Fri, Dec 07, 2018 at 10:28:09AM +0100, Paul B Mahol wrote:
> >> On 12/7/18, Michael Niedermayer <michael@niedermayer.cc> wrote:
> >> > On Wed, Dec 05, 2018 at 09:22:48PM +0100, Paul B Mahol wrote:
> >> >> Signed-off-by: Paul B Mahol <onemda@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

     
[...]
Paul B Mahol Dec. 7, 2018, 12:15 p.m.
On 12/7/18, Michael Niedermayer <michael@niedermayer.cc> wrote:
> On Fri, Dec 07, 2018 at 11:21:57AM +0100, Paul B Mahol wrote:
>> On 12/7/18, Michael Niedermayer <michael@niedermayer.cc> wrote:
>> > On Fri, Dec 07, 2018 at 10:28:09AM +0100, Paul B Mahol wrote:
>> >> On 12/7/18, Michael Niedermayer <michael@niedermayer.cc> wrote:
>> >> > On Wed, Dec 05, 2018 at 09:22:48PM +0100, Paul B Mahol wrote:
>> >> >> Signed-off-by: Paul B Mahol <onemda@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.
Hendrik Leppkes Dec. 7, 2018, 12:43 p.m.
On Fri, Dec 7, 2018 at 1:15 PM Paul B Mahol <onemda@gmail.com> wrote:
>
> On 12/7/18, Michael Niedermayer <michael@niedermayer.cc> wrote:
> > On Fri, Dec 07, 2018 at 11:21:57AM +0100, Paul B Mahol wrote:
> >> On 12/7/18, Michael Niedermayer <michael@niedermayer.cc> wrote:
> >> > On Fri, Dec 07, 2018 at 10:28:09AM +0100, Paul B Mahol wrote:
> >> >> On 12/7/18, Michael Niedermayer <michael@niedermayer.cc> wrote:
> >> >> > On Wed, Dec 05, 2018 at 09:22:48PM +0100, Paul B Mahol wrote:
> >> >> >> Signed-off-by: Paul B Mahol <onemda@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
Michael Niedermayer Dec. 7, 2018, 4:49 p.m.
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@gmail.com> wrote:
> >
> > On 12/7/18, Michael Niedermayer <michael@niedermayer.cc> wrote:
> > > On Fri, Dec 07, 2018 at 11:21:57AM +0100, Paul B Mahol wrote:
> > >> On 12/7/18, Michael Niedermayer <michael@niedermayer.cc> wrote:
> > >> > On Fri, Dec 07, 2018 at 10:28:09AM +0100, Paul B Mahol wrote:
> > >> >> On 12/7/18, Michael Niedermayer <michael@niedermayer.cc> wrote:
> > >> >> > On Wed, Dec 05, 2018 at 09:22:48PM +0100, Paul B Mahol wrote:
> > >> >> >> Signed-off-by: Paul B Mahol <onemda@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/


[...]

Patch hide | download patch | download mbox

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)
 
     avctx->bits_per_raw_sample = 10;
 
-    switch (avctx->codec_tag) {
-    case MKTAG('a','p','c','o'):
-        avctx->profile = FF_PROFILE_PRORES_PROXY;
-        break;
-    case MKTAG('a','p','c','s'):
-        avctx->profile = FF_PROFILE_PRORES_LT;
-        break;
-    case MKTAG('a','p','c','n'):
-        avctx->profile = FF_PROFILE_PRORES_STANDARD;
-        break;
-    case MKTAG('a','p','c','h'):
-        avctx->profile = FF_PROFILE_PRORES_HQ;
-        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_UNKNOWN) {
+        switch (avctx->codec_tag) {
+        case MKTAG('a','p','c','o'):
+            avctx->profile = FF_PROFILE_PRORES_PROXY;
+            break;
+        case MKTAG('a','p','c','s'):
+            avctx->profile = FF_PROFILE_PRORES_LT;
+            break;
+        case MKTAG('a','p','c','n'):
+            avctx->profile = FF_PROFILE_PRORES_STANDARD;
+            break;
+        case MKTAG('a','p','c','h'):
+            avctx->profile = FF_PROFILE_PRORES_HQ;
+            break;
+        case MKTAG('a','p','4','h'):
+            avctx->profile = FF_PROFILE_PRORES_4444;
+            break;
+        case MKTAG('a','p','4','x'):
+            avctx->profile = FF_PROFILE_PRORES_XQ;
+            break;
+        default:
+            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;
+
     if (avctx->bits_per_raw_sample == 10) {
         av_log(avctx, AV_LOG_DEBUG, "Auto bitdepth precision. Use 10b decoding based on codec tag");
     } else { /* 12b */