| Message ID | 20181205202248.16504-1-onemda@gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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
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.
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
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?
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 [...]
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.
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
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/ [...]
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 */
Signed-off-by: Paul B Mahol <onemda@gmail.com> --- libavcodec/proresdec2.c | 51 ++++++++++++++++++++++------------------- 1 file changed, 27 insertions(+), 24 deletions(-)