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 <one...@gmail.com> wrote: > > > > 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. > > 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
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel