Re: [FFmpeg-devel] [PATCH 1/4] avcodec/proresdec2: change profile only if it is unknown
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 wrote:
> >
> > On 12/7/18, Michael Niedermayer wrote:
> > > On Fri, Dec 07, 2018 at 11:21:57AM +0100, Paul B Mahol wrote:
> > >> On 12/7/18, Michael Niedermayer wrote:
> > >> > On Fri, Dec 07, 2018 at 10:28:09AM +0100, Paul B Mahol wrote:
> > >> >> On 12/7/18, Michael Niedermayer wrote:
> > >> >> > On Wed, Dec 05, 2018 at 09:22:48PM +0100, Paul B Mahol wrote:
> > >> >> >> Signed-off-by: Paul B Mahol
> > >> >> >> ---
> > >> >> >> 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_;
> > >> >> >>-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_)
> > >> >> >>+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
Re: [FFmpeg-devel] [PATCH 1/4] avcodec/proresdec2: change profile only if it is unknown
On Fri, Dec 7, 2018 at 1:15 PM Paul B Mahol wrote:
>
> On 12/7/18, Michael Niedermayer wrote:
> > On Fri, Dec 07, 2018 at 11:21:57AM +0100, Paul B Mahol wrote:
> >> On 12/7/18, Michael Niedermayer wrote:
> >> > On Fri, Dec 07, 2018 at 10:28:09AM +0100, Paul B Mahol wrote:
> >> >> On 12/7/18, Michael Niedermayer wrote:
> >> >> > On Wed, Dec 05, 2018 at 09:22:48PM +0100, Paul B Mahol wrote:
> >> >> >> Signed-off-by: Paul B Mahol
> >> >> >> ---
> >> >> >> 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_;
> >> >> >>-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_)
> >> >> >>+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:
Re: [FFmpeg-devel] [PATCH 1/4] avcodec/proresdec2: change profile only if it is unknown
On 12/7/18, Michael Niedermayer wrote:
> On Fri, Dec 07, 2018 at 11:21:57AM +0100, Paul B Mahol wrote:
>> On 12/7/18, Michael Niedermayer wrote:
>> > On Fri, Dec 07, 2018 at 10:28:09AM +0100, Paul B Mahol wrote:
>> >> On 12/7/18, Michael Niedermayer wrote:
>> >> > On Wed, Dec 05, 2018 at 09:22:48PM +0100, Paul B Mahol wrote:
>> >> >> Signed-off-by: Paul B Mahol
>> >> >> ---
>> >> >> 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_;
>> >> >>-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_)
>> >> >>+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
Re: [FFmpeg-devel] [PATCH 1/4] avcodec/proresdec2: change profile only if it is unknown
On Fri, Dec 07, 2018 at 11:21:57AM +0100, Paul B Mahol wrote:
> On 12/7/18, Michael Niedermayer wrote:
> > On Fri, Dec 07, 2018 at 10:28:09AM +0100, Paul B Mahol wrote:
> >> On 12/7/18, Michael Niedermayer wrote:
> >> > On Wed, Dec 05, 2018 at 09:22:48PM +0100, Paul B Mahol wrote:
> >> >> Signed-off-by: Paul B Mahol
> >> >> ---
> >> >> 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_;
> >> >>-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_)
> >> >>+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
Re: [FFmpeg-devel] [PATCH 1/4] avcodec/proresdec2: change profile only if it is unknown
On 12/7/18, Michael Niedermayer wrote:
> On Fri, Dec 07, 2018 at 10:28:09AM +0100, Paul B Mahol wrote:
>> On 12/7/18, Michael Niedermayer wrote:
>> > On Wed, Dec 05, 2018 at 09:22:48PM +0100, Paul B Mahol wrote:
>> >> Signed-off-by: Paul B Mahol
>> >> ---
>> >> 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_;
>> >>-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_)
>> >>+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?
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/4] avcodec/proresdec2: change profile only if it is unknown
On Fri, Dec 07, 2018 at 10:28:09AM +0100, Paul B Mahol wrote:
> On 12/7/18, Michael Niedermayer wrote:
> > On Wed, Dec 05, 2018 at 09:22:48PM +0100, Paul B Mahol wrote:
> >> Signed-off-by: Paul B Mahol
> >> ---
> >> 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_;
> >>-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_)
> >>+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
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Breaking DRM is a little like attempting to break through a door even
though the window is wide open and the only thing in the house is a bunch
of things you dont want and which you would get tomorrow for free anyway
signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/4] avcodec/proresdec2: change profile only if it is unknown
On 12/7/18, Michael Niedermayer wrote:
> On Wed, Dec 05, 2018 at 09:22:48PM +0100, Paul B Mahol wrote:
>> Signed-off-by: Paul B Mahol
>> ---
>> 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_;
>>-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_)
>>+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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/4] avcodec/proresdec2: change profile only if it is unknown
On Wed, Dec 05, 2018 at 09:22:48PM +0100, Paul B Mahol wrote:
> Signed-off-by: Paul B Mahol
> ---
> 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_;
>-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_)
>+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
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
During times of universal deceit, telling the truth becomes a
revolutionary act. -- George Orwell
signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/4] avcodec/proresdec2: change profile only if it is unknown
2018-12-05 19:00 GMT+01:00, Gregor Riepl : > There seems to be an indentation issue here: This is intended to allow an actual review. Carl Eugen ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/4] avcodec/proresdec2: change profile only if it is unknown
2018-12-05 21:22 GMT+01:00, Paul B Mahol :
> Signed-off-by: Paul B Mahol
> ---
> 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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 1/4] avcodec/proresdec2: change profile only if it is unknown
Signed-off-by: Paul B Mahol
---
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) {
-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_;
-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_;
+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_)
+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 */
--
2.17.1
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/4] avcodec/proresdec2: change profile only if it is unknown
On Wed, Dec 05, 2018 at 06:52:06PM +0100, Paul B Mahol wrote: > Signed-off-by: Paul B Mahol > --- > libavcodec/proresdec2.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) breaks make fate-prores-alpha_skip TESTprores-alpha_skip --- ./tests/ref/fate/prores-alpha_skip 2018-12-04 23:28:22.177830519 +0100 +++ tests/data/fate/prores-alpha_skip 2018-12-05 20:27:52.523422579 +0100 @@ -3,5 +3,5 @@ #codec_id 0: rawvideo #dimensions 0: 1920x1080 #sar 0: 0/1 -0, 0, 0,1, 12441600, 0x65e009b8 -0, 1, 1,1, 12441600, 0x65e009b8 +0, 0, 0,1, 12441600, 0xf06f2ff5 +0, 1, 1,1, 12441600, 0xf06f2ff5 Test prores-alpha_skip failed. Look at tests/data/fate/prores-alpha_skip.err for details. make: *** [fate-prores-alpha_skip] Error 1 [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Its not that you shouldnt use gotos but rather that you should write readable code and code with gotos often but not always is less readable signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/4] avcodec/proresdec2: change profile only if it is unknown
There seems to be an indentation issue here:
> +if (avctx->profile == FF_PROFILE_UNKNOWN) {
> switch (avctx->codec_tag) {
> case MKTAG('a','p','c','o'):
and here:
> av_log(avctx, AV_LOG_WARNING, "Unknown prores profile %d\n",
> avctx->codec_tag);
> }
> +}
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 1/4] avcodec/proresdec2: change profile only if it is unknown
Signed-off-by: Paul B Mahol
---
libavcodec/proresdec2.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/libavcodec/proresdec2.c b/libavcodec/proresdec2.c
index 8581d797fb..f715b86aad 100644
--- a/libavcodec/proresdec2.c
+++ b/libavcodec/proresdec2.c
@@ -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;
@@ -162,9 +163,9 @@ static av_cold int decode_init(AVCodecContext *avctx)
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->bits_per_raw_sample == 10) {
av_log(avctx, AV_LOG_DEBUG, "Auto bitdepth precision. Use 10b decoding
based on codec tag");
--
2.17.1
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
