On 12/7/18, Michael Niedermayer <mich...@niedermayer.cc> wrote: > On Fri, Dec 07, 2018 at 11:21:57AM +0100, Paul B Mahol wrote: >> On 12/7/18, Michael Niedermayer <mich...@niedermayer.cc> wrote: >> > On Fri, Dec 07, 2018 at 10:28:09AM +0100, Paul B Mahol wrote: >> >> On 12/7/18, Michael Niedermayer <mich...@niedermayer.cc> wrote: >> >> > On Wed, Dec 05, 2018 at 09:22:48PM +0100, Paul B Mahol wrote: >> >> >> Signed-off-by: Paul B Mahol <one...@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. _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel