[FFmpeg-devel,v3,1/2] avcodec/codec_par: Add codec properties field to AVCodecParameters

This fixes incorrect display of closed_captions property in ffprobe.

Repro Example:
ffprobe -show_entries stream=closed_captions:disposition=:side_data=  "http://streams.videolan.org/streams/ts/CC/NewsStream-608-ac3.ts"

While the codec string included "Closed Captions",
the stream data is showed: closed_captions=0

The test ref was incorrect as the test media file actually does
have cc, which is fixed by this commit, so the test ref needs
to be updated.

Signed-off-by: softworkz <softworkz@hotmail.com>
---
v3: Moved test update to the right (this) commit

 doc/APIchanges          | 3 +++
 libavcodec/codec_par.c  | 2 ++
 libavcodec/codec_par.h  | 4 ++++
 libavcodec/version.h    | 2 +-
 tests/ref/fate/ts-demux | 2 +-
 5 files changed, 11 insertions(+), 2 deletions(-)

 stream|index=1|codec_name=ac3|profile=unknown|codec_type=audio|codec_tag_string=[4][0][0][0]|codec_tag=0x0004|sample_fmt=fltp|sample_rate=48000|channels=6|channel_layout=5.1(side)|bits_per_sample=0|id=0x34|r_frame_rate=0/0|avg_frame_rate=0/0|time_base=1/90000|start_pts=3912633305|start_time=43473.703389|duration_ts=14400|duration=0.160000|bit_rate=384000|max_bit_rate=N/A|bits_per_raw_sample=N/A|nb_frames=N/A|nb_read_frames=N/A|nb_read_packets=5|disposition:default=0|disposition:dub=0|disposition:original=0|disposition:comment=0|disposition:lyrics=0|disposition:karaoke=0|disposition:forced=0|disposition:hearing_impaired=0|disposition:visual_impaired=0|disposition:clean_effects=0|disposition:attached_pic=0|disposition:timed_thumbnails=0|disposition:captions=0|disposition:descriptions=0|disposition:metadata=0|disposition:dependent=0|disposition:still_image=0|tag:language=eng
 stream|index=2|codec_name=ac3|profile=unknown|codec_type=audio|codec_tag_string=[4][0][0][0]|codec_tag=0x0004|sample_fmt=fltp|sample_rate=48000|channels=2|channel_layout=stereo|bits_per_sample=0|id=0x35|r_frame_rate=0/0|avg_frame_rate=0/0|time_base=1/90000|start_pts=3912634060|start_time=43473.711778|duration_ts=14400|duration=0.160000|bit_rate=192000|max_bit_rate=N/A|bits_per_raw_sample=N/A|nb_frames=N/A|nb_read_frames=N/A|nb_read_packets=5|disposition:default=0|disposition:dub=0|disposition:original=0|disposition:comment=0|disposition:lyrics=0|disposition:karaoke=0|disposition:forced=0|disposition:hearing_impaired=0|disposition:visual_impaired=0|disposition:clean_effects=0|disposition:attached_pic=0|disposition:timed_thumbnails=0|disposition:captions=0|disposition:descriptions=0|disposition:metadata=0|disposition:dependent=0|disposition:still_image=0|tag:language=es

On Wed, Oct 6, 2021 at 8:45 AM Soft Works <softworkz@hotmail.com> wrote:
> diff --git a/libavcodec/codec_par.h b/libavcodec/codec_par.h
> index 10cf79dff1..42ed8deb13 100644
> --- a/libavcodec/codec_par.h
> +++ b/libavcodec/codec_par.h
> @@ -198,6 +198,10 @@ typedef struct AVCodecParameters {
>       * Audio only. Number of samples to skip after a discontinuity.
>       */
>      int seek_preroll;
> +    /**
> +     * Codec properties of the stream that gets decoded
> +     */
> +    unsigned properties;
>  } AVCodecParameters;
>

This field is severly underspecified/underdocumented. I realize you
just copied it, but if I'm looking at this without pre-existing
knowledge, then I have absolutely no idea what it might be for, or
what kind of values go in there. The old field had the defines of the
possible bits right there for context - this doesn't have this, so it
would definitely benefit from added documentation.

- Hendrik

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Hendrik Leppkes
> Sent: Wednesday, October 6, 2021 8:57 AM
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH v3 1/2] avcodec/codec_par: Add
> codec properties field to AVCodecParameters
> 
> On Wed, Oct 6, 2021 at 8:45 AM Soft Works <softworkz@hotmail.com>
> wrote:
> > diff --git a/libavcodec/codec_par.h b/libavcodec/codec_par.h
> > index 10cf79dff1..42ed8deb13 100644
> > --- a/libavcodec/codec_par.h
> > +++ b/libavcodec/codec_par.h
> > @@ -198,6 +198,10 @@ typedef struct AVCodecParameters {
> >       * Audio only. Number of samples to skip after a
> discontinuity.
> >       */
> >      int seek_preroll;
> > +    /**
> > +     * Codec properties of the stream that gets decoded
> > +     */
> > +    unsigned properties;
> >  } AVCodecParameters;
> >
> 
> This field is severly underspecified/underdocumented. I realize you
> just copied it, but if I'm looking at this without pre-existing
> knowledge, then I have absolutely no idea what it might be for, or
> what kind of values go in there. The old field had the defines of the
> possible bits right there for context - this doesn't have this, so it
> would definitely benefit from added documentation.

Hello Hendrik,

that's right, but what would you suggest? We can't duplicate the defines,
though I could add a line in the help text like 

    /**
     * Codec properties of the stream that gets decoded.
     * Corresponds to @ref AVCodecContext.properties "properties".
     */
    unsigned properties;

Would that be OK?

Thanks,
sw

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Soft Works
> Sent: Wednesday, October 6, 2021 8:45 AM
> To: ffmpeg-devel@ffmpeg.org
> Subject: [FFmpeg-devel] [PATCH v3 1/2] avcodec/codec_par: Add codec
> properties field to AVCodecParameters
> 
> This fixes incorrect display of closed_captions property in ffprobe.
> 
> Repro Example:
> ffprobe -show_entries stream=closed_captions:disposition=:side_data=
> "http://streams.videolan.org/streams/ts/CC/NewsStream-608-ac3.ts"
> 
> While the codec string included "Closed Captions",
> the stream data is showed: closed_captions=0
> 
> The test ref was incorrect as the test media file actually does
> have cc, which is fixed by this commit, so the test ref needs
> to be updated.
> 
> Signed-off-by: softworkz <softworkz@hotmail.com>
> ---
> v3: Moved test update to the right (this) commit
> 
>  doc/APIchanges          | 3 +++
>  libavcodec/codec_par.c  | 2 ++
>  libavcodec/codec_par.h  | 4 ++++
>  libavcodec/version.h    | 2 +-
>  tests/ref/fate/ts-demux | 2 +-
>  5 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/doc/APIchanges b/doc/APIchanges
> index 7b267a79ac..2be3303efa 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -14,6 +14,9 @@ libavutil:     2021-04-27
> 
>  API changes, most recent first:
> 
> +2021-10-07 - xxxxxxxxxx - lavc 59.11.100 - codec_par.h
> +  Add codec properties field to AVCodecParameters
> +
>  2021-09-21 - xxxxxxxxxx - lavu 57.7.100 - pixfmt.h
>    Add AV_PIX_FMT_X2BGR10.
> 
> diff --git a/libavcodec/codec_par.c b/libavcodec/codec_par.c
> index 1a5168a04b..f6c13f7d11 100644
> --- a/libavcodec/codec_par.c
> +++ b/libavcodec/codec_par.c
> @@ -101,6 +101,7 @@ int
> avcodec_parameters_from_context(AVCodecParameters *par,
>      par->bits_per_raw_sample   = codec->bits_per_raw_sample;
>      par->profile               = codec->profile;
>      par->level                 = codec->level;
> +    par->properties            = codec->properties;
> 
>      switch (par->codec_type) {
>      case AVMEDIA_TYPE_VIDEO:
> @@ -156,6 +157,7 @@ int avcodec_parameters_to_context(AVCodecContext
> *codec,
>      codec->bits_per_raw_sample   = par->bits_per_raw_sample;
>      codec->profile               = par->profile;
>      codec->level                 = par->level;
> +    codec->properties            = par->properties;
> 
>      switch (par->codec_type) {
>      case AVMEDIA_TYPE_VIDEO:
> diff --git a/libavcodec/codec_par.h b/libavcodec/codec_par.h
> index 10cf79dff1..42ed8deb13 100644
> --- a/libavcodec/codec_par.h
> +++ b/libavcodec/codec_par.h
> @@ -198,6 +198,10 @@ typedef struct AVCodecParameters {
>       * Audio only. Number of samples to skip after a discontinuity.
>       */
>      int seek_preroll;
> +    /**
> +     * Codec properties of the stream that gets decoded
> +     */
> +    unsigned properties;
>  } AVCodecParameters;
> 
>  /**
> diff --git a/libavcodec/version.h b/libavcodec/version.h
> index 4bd22f7e93..1c28fd0be5 100644
> --- a/libavcodec/version.h
> +++ b/libavcodec/version.h
> @@ -28,7 +28,7 @@
>  #include "libavutil/version.h"
> 
>  #define LIBAVCODEC_VERSION_MAJOR  59
> -#define LIBAVCODEC_VERSION_MINOR  10
> +#define LIBAVCODEC_VERSION_MINOR  11
>  #define LIBAVCODEC_VERSION_MICRO 100
> 
>  #define LIBAVCODEC_VERSION_INT
> AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
> diff --git a/tests/ref/fate/ts-demux b/tests/ref/fate/ts-demux
> index 8e7a81da41..1d1382cf37 100644
> --- a/tests/ref/fate/ts-demux
> +++ b/tests/ref/fate/ts-demux
> @@ -41,7 +41,7 @@
> packet|codec_type=audio|stream_index=2|pts=3912642700|pts_time=43473.
> 807778|dts=
> 
> packet|codec_type=video|stream_index=0|pts=3912686363|pts_time=43474.
> 292922|dts=3912686363|dts_time=43474.292922|duration=1501|duration_ti
> me=0.016678|size=4944|pos=506660|flags=__|data_hash=CRC32:54a86cbb
> 
> packet|codec_type=audio|stream_index=1|pts=3912644825|pts_time=43473.
> 831389|dts=3912644825|dts_time=43473.831389|duration=2880|duration_ti
> me=0.032000|size=906|pos=474888|flags=K_|data_hash=CRC32:0893d398
> 
> packet|codec_type=audio|stream_index=2|pts=3912645580|pts_time=43473.
> 839778|dts=3912645580|dts_time=43473.839778|duration=2880|duration_ti
> me=0.032000|size=354|pos=491808|flags=K_|data_hash=CRC32:f5963fa6
> -
> stream|index=0|codec_name=mpeg2video|profile=4|codec_type=video|codec
> _tag_string=[2][0][0][0]|codec_tag=0x0002|width=1280|height=720|coded
> _width=0|coded_height=0|closed_captions=0|film_grain=0|has_b_frames=1
> |sample_aspect_ratio=1:1|display_aspect_ratio=16:9|pix_fmt=yuv420p|le
> vel=4|color_range=tv|color_space=unknown|color_transfer=unknown|color
> _primaries=unknown|chroma_location=left|field_order=progressive|refs=
> 1|id=0x31|r_frame_rate=60000/1001|avg_frame_rate=60000/1001|time_base
> =1/90000|start_pts=3912669846|start_time=43474.109400|duration_ts=195
> 19|duration=0.216878|bit_rate=15000000|max_bit_rate=N/A|bits_per_raw_
> sample=N/A|nb_frames=N/A|nb_read_frames=N/A|nb_read_packets=15|extrad
> ata_hash=CRC32:53134fa8|disposition:default=0|disposition:dub=0|dispo
> sition:original=0|disposition:comment=0|disposition:lyrics=0|disposit
> ion:karaoke=0|disposition:forced=0|disposition:hearing_impaired=0|dis
> position:visual_impaired=0|disposition:clean_effects=0|disposition:at
> tached_pic=0|disposition:timed_

Line break was inserted here!

> thumbnails=0|disposition:captions=0|disposition:descriptions=0|dispos
> ition:metadata=0|disposition:dependent=0|disposition:still_image=0
> +stream|index=0|codec_name=mpeg2video|profile=4|codec_type=video|code
> c_tag_string=[2][0][0][0]|codec_tag=0x0002|width=1280|height=720|code
> d_width=0|coded_height=0|closed_captions=1|film_grain=0|has_b_frames=
> 1|sample_aspect_ratio=1:1|display_aspect_ratio=16:9|pix_fmt=yuv420p|l
> evel=4|color_range=tv|color_space=unknown|color_transfer=unknown|colo
> r_primaries=unknown|chroma_location=left|field_order=progressive|refs
> =1|id=0x31|r_frame_rate=60000/1001|avg_frame_rate=60000/1001|time_bas
> e=1/90000|start_pts=3912669846|start_time=43474.109400|duration_ts=19
> 519|duration=0.216878|bit_rate=15000000|max_bit_rate=N/A|bits_per_raw
> _sample=N/A|nb_frames=N/A|nb_read_frames=N/A|nb_read_packets=15|extra
> data_hash=CRC32:53134fa8|disposition:default=0|disposition:dub=0|disp
> osition:original=0|disposition:comment=0|disposition:lyrics=0|disposi
> tion:karaoke=0|disposition:forced=0|disposition:hearing_impaired=0|di
> sposition:visual_impaired=0|disposition:clean_effects=0|disposition:a
> ttached_pic=0|disposition:timed_

Line break was inserted here!

> thumbnails=0|disposition:captions=0|disposition:descriptions=0|dispos
> ition:metadata=0|disposition:dependent=0|disposition:still_image=0
>  side_data|side_data_type=CPB
> properties|max_bitrate=15000000|min_bitrate=0|avg_bitrate=0|buffer_si
> ze=9781248|vbv_delay=-1
> 
> stream|index=1|codec_name=ac3|profile=unknown|codec_type=audio|codec_
> tag_string=[4][0][0][0]|codec_tag=0x0004|sample_fmt=fltp|sample_rate=
> 48000|channels=6|channel_layout=5.1(side)|bits_per_sample=0|id=0x34|r
> _frame_rate=0/0|avg_frame_rate=0/0|time_base=1/90000|start_pts=391263
> 3305|start_time=43473.703389|duration_ts=14400|duration=0.160000|bit_
> rate=384000|max_bit_rate=N/A|bits_per_raw_sample=N/A|nb_frames=N/A|nb
> _read_frames=N/A|nb_read_packets=5|disposition:default=0|disposition:
> dub=0|disposition:original=0|disposition:comment=0|disposition:lyrics
> =0|disposition:karaoke=0|disposition:forced=0|disposition:hearing_imp
> aired=0|disposition:visual_impaired=0|disposition:clean_effects=0|dis
> position:attached_pic=0|disposition:timed_thumbnails=0|disposition:ca
> ptions=0|disposition:descriptions=0|disposition:metadata=0|dispositio
> n:dependent=0|disposition:still_image=0|tag:language=eng
> 
> stream|index=2|codec_name=ac3|profile=unknown|codec_type=audio|codec_
> tag_string=[4][0][0][0]|codec_tag=0x0004|sample_fmt=fltp|sample_rate=
> 48000|channels=2|channel_layout=stereo|bits_per_sample=0|id=0x35|r_fr
> ame_rate=0/0|avg_frame_rate=0/0|time_base=1/90000|start_pts=391263406
> 0|start_time=43473.711778|duration_ts=14400|duration=0.160000|bit_rat
> e=192000|max_bit_rate=N/A|bits_per_raw_sample=N/A|nb_frames=N/A|nb_re
> ad_frames=N/A|nb_read_packets=5|disposition:default=0|disposition:dub
> =0|disposition:original=0|disposition:comment=0|disposition:lyrics=0|
> disposition:karaoke=0|disposition:forced=0|disposition:hearing_impair
> ed=0|disposition:visual_impaired=0|disposition:clean_effects=0|dispos
> ition:attached_pic=0|disposition:timed_thumbnails=0|disposition:capti
> ons=0|disposition:descriptions=0|disposition:metadata=0|disposition:d
> ependent=0|disposition:still_image=0|tag:language=es
> --

I used to think that it's caused by Outlook, but I had BCCed this
patch to 3 other e-mail accounts and at all of them, the message has
arrived without those erroneous line breaks added.

Is it the mailing list which adds those line breaks?

Kind regards,
softworkz

Soft Works:
> 
> 
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
>> Hendrik Leppkes
>> Sent: Wednesday, October 6, 2021 8:57 AM
>> To: FFmpeg development discussions and patches <ffmpeg-
>> devel@ffmpeg.org>
>> Subject: Re: [FFmpeg-devel] [PATCH v3 1/2] avcodec/codec_par: Add
>> codec properties field to AVCodecParameters
>>
>> On Wed, Oct 6, 2021 at 8:45 AM Soft Works <softworkz@hotmail.com>
>> wrote:
>>> diff --git a/libavcodec/codec_par.h b/libavcodec/codec_par.h
>>> index 10cf79dff1..42ed8deb13 100644
>>> --- a/libavcodec/codec_par.h
>>> +++ b/libavcodec/codec_par.h
>>> @@ -198,6 +198,10 @@ typedef struct AVCodecParameters {
>>>       * Audio only. Number of samples to skip after a
>> discontinuity.
>>>       */
>>>      int seek_preroll;
>>> +    /**
>>> +     * Codec properties of the stream that gets decoded
>>> +     */
>>> +    unsigned properties;
>>>  } AVCodecParameters;
>>>
>>
>> This field is severly underspecified/underdocumented. I realize you
>> just copied it, but if I'm looking at this without pre-existing
>> knowledge, then I have absolutely no idea what it might be for, or
>> what kind of values go in there. The old field had the defines of the
>> possible bits right there for context - this doesn't have this, so it
>> would definitely benefit from added documentation.
> 
> Hello Hendrik,
> 
> that's right, but what would you suggest? We can't duplicate the defines,
> though I could add a line in the help text like 
> 
>     /**
>      * Codec properties of the stream that gets decoded.
>      * Corresponds to @ref AVCodecContext.properties "properties".
>      */
>     unsigned properties;
> 
> Would that be OK?
> 

The defines could be moved to defs.h (while just at it, we could also
add properly prefixed alternatives and deprecate the FF_ ones).
The copying that you are adding in  avcodec_parameters_from_context is
problematic, as the relevant AVCodecContext field is marked as "set by
libavcodec" for decoding, which means that decoders expect it to be
blank initially. Imagine a scenario where the beginning (that gets read
in avformat_find_stream_info()) of an input stream has e.g. embedded cc
subtitles, but the part that lateron gets fed to the decoder does not.
If the decoder has been initialized from the AVStream's
AVCodecParameters, then said decoder would still indicate that there are
closed captions.* IIRC Anton already mentioned that this is actually a
frame/packet property, not a stream property.

- Andreas

*: It seems that the properties field is treated inconsistently:
LOSSLESS and CLOSED_CAPTIONS are only ever set, never unset, FILM_GRAIN
is also unset.

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Andreas Rheinhardt
> Sent: Wednesday, October 6, 2021 11:30 AM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v3 1/2] avcodec/codec_par: Add
> codec properties field to AVCodecParameters
> 
> Soft Works:
> >
> >
> >> -----Original Message-----
> >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> >> Hendrik Leppkes
> >> Sent: Wednesday, October 6, 2021 8:57 AM
> >> To: FFmpeg development discussions and patches <ffmpeg-
> >> devel@ffmpeg.org>
> >> Subject: Re: [FFmpeg-devel] [PATCH v3 1/2] avcodec/codec_par: Add
> >> codec properties field to AVCodecParameters
> >>
> >> On Wed, Oct 6, 2021 at 8:45 AM Soft Works <softworkz@hotmail.com>
> >> wrote:
> >>> diff --git a/libavcodec/codec_par.h b/libavcodec/codec_par.h
> >>> index 10cf79dff1..42ed8deb13 100644
> >>> --- a/libavcodec/codec_par.h
> >>> +++ b/libavcodec/codec_par.h
> >>> @@ -198,6 +198,10 @@ typedef struct AVCodecParameters {
> >>>       * Audio only. Number of samples to skip after a
> >> discontinuity.
> >>>       */
> >>>      int seek_preroll;
> >>> +    /**
> >>> +     * Codec properties of the stream that gets decoded
> >>> +     */
> >>> +    unsigned properties;
> >>>  } AVCodecParameters;
> >>>
> >>
> >> This field is severly underspecified/underdocumented. I realize
> you
> >> just copied it, but if I'm looking at this without pre-existing
> >> knowledge, then I have absolutely no idea what it might be for, or
> >> what kind of values go in there. The old field had the defines of
> the
> >> possible bits right there for context - this doesn't have this, so
> it
> >> would definitely benefit from added documentation.
> >
> > Hello Hendrik,
> >
> > that's right, but what would you suggest? We can't duplicate the
> defines,
> > though I could add a line in the help text like
> >
> >     /**
> >      * Codec properties of the stream that gets decoded.
> >      * Corresponds to @ref AVCodecContext.properties "properties".
> >      */
> >     unsigned properties;
> >
> > Would that be OK?
> >
> 
> The defines could be moved to defs.h (while just at it, we could also
> add properly prefixed alternatives and deprecate the FF_ ones).
> The copying that you are adding in  avcodec_parameters_from_context
> is
> problematic, as the relevant AVCodecContext field is marked as "set
> by
> libavcodec" for decoding, which means that decoders expect it to be
> blank initially. 

Why do you think that decoders would expect it to be blank initially?

What about the other fields then, like format, profile, level, width,
height, etc. - these are copied in the same way.
Are you saying that decoders expect all other fields to be filled but 
the properties field to be zero?

Also, the docs say that it's SET by decoders, not READ by decoders,
which means that no decoder could rely on this field. There's also
no undefined-state defined (like -1), which means that a decoder 
couldn't know whether a 0-value indicates a valid value or a not-yet
initialized value.

> Imagine a scenario where the beginning (that gets
> read
> in avformat_find_stream_info()) of an input stream has e.g. embedded
> cc
> subtitles, but the part that lateron gets fed to the decoder does
> not.
> If the decoder has been initialized from the AVStream's
> AVCodecParameters, then said decoder would still indicate that there
> are
> closed captions.* IIRC Anton already mentioned that this is actually
> a
> frame/packet property, not a stream property.

That statement was not thought through. Of course it IS a frame 
property as well, but there's no difference to other properties like
width, height or aspect ratio:

- as a frame property, it is indicating whether a frame carries 
  closed caption data
- as a stream property, it indicates the summary result from the 
  probing process

Just the same like some other fields in this struct.

Kind regards,
softworkz

Soft Works:
> 
> 
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
>> Andreas Rheinhardt
>> Sent: Wednesday, October 6, 2021 11:30 AM
>> To: ffmpeg-devel@ffmpeg.org
>> Subject: Re: [FFmpeg-devel] [PATCH v3 1/2] avcodec/codec_par: Add
>> codec properties field to AVCodecParameters
>>
>> Soft Works:
>>>
>>>
>>>> -----Original Message-----
>>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
>>>> Hendrik Leppkes
>>>> Sent: Wednesday, October 6, 2021 8:57 AM
>>>> To: FFmpeg development discussions and patches <ffmpeg-
>>>> devel@ffmpeg.org>
>>>> Subject: Re: [FFmpeg-devel] [PATCH v3 1/2] avcodec/codec_par: Add
>>>> codec properties field to AVCodecParameters
>>>>
>>>> On Wed, Oct 6, 2021 at 8:45 AM Soft Works <softworkz@hotmail.com>
>>>> wrote:
>>>>> diff --git a/libavcodec/codec_par.h b/libavcodec/codec_par.h
>>>>> index 10cf79dff1..42ed8deb13 100644
>>>>> --- a/libavcodec/codec_par.h
>>>>> +++ b/libavcodec/codec_par.h
>>>>> @@ -198,6 +198,10 @@ typedef struct AVCodecParameters {
>>>>>       * Audio only. Number of samples to skip after a
>>>> discontinuity.
>>>>>       */
>>>>>      int seek_preroll;
>>>>> +    /**
>>>>> +     * Codec properties of the stream that gets decoded
>>>>> +     */
>>>>> +    unsigned properties;
>>>>>  } AVCodecParameters;
>>>>>
>>>>
>>>> This field is severly underspecified/underdocumented. I realize
>> you
>>>> just copied it, but if I'm looking at this without pre-existing
>>>> knowledge, then I have absolutely no idea what it might be for, or
>>>> what kind of values go in there. The old field had the defines of
>> the
>>>> possible bits right there for context - this doesn't have this, so
>> it
>>>> would definitely benefit from added documentation.
>>>
>>> Hello Hendrik,
>>>
>>> that's right, but what would you suggest? We can't duplicate the
>> defines,
>>> though I could add a line in the help text like
>>>
>>>     /**
>>>      * Codec properties of the stream that gets decoded.
>>>      * Corresponds to @ref AVCodecContext.properties "properties".
>>>      */
>>>     unsigned properties;
>>>
>>> Would that be OK?
>>>
>>
>> The defines could be moved to defs.h (while just at it, we could also
>> add properly prefixed alternatives and deprecate the FF_ ones).
>> The copying that you are adding in  avcodec_parameters_from_context
>> is
>> problematic, as the relevant AVCodecContext field is marked as "set
>> by
>> libavcodec" for decoding, which means that decoders expect it to be
>> blank initially. 
> 
> Why do you think that decoders would expect it to be blank initially?
> 
> What about the other fields then, like format, profile, level, width,
> height, etc. - these are copied in the same way.
> Are you saying that decoders expect all other fields to be filled but 
> the properties field to be zero?
> 

Of course not. E.g. the documentation of the dimension explicitly says
so: "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."
The semantics of profile and level are closer, yet the difference is
that if the decoder sets these fields, it overwrites any garbage it may
have had; this is not so with a bitfield like properties.

> Also, the docs say that it's SET by decoders, not READ by decoders,
> which means that no decoder could rely on this field. There's also
> no undefined-state defined (like -1), which means that a decoder 
> couldn't know whether a 0-value indicates a valid value or a not-yet
> initialized value.
> 

You completely misunderstood what my concern was about: In the scenario
described above an API user querying the properties field would get the
information that this stream contains CC data; even when it doesn't.

And your other claims are also wrong: A decoder may read anything from
an AVCodecContext or from the AVPackets and AVFrames with the exception
of data that is opaque to it. And if a field is described as
out-of-touch by the user, then the codec may rely on it not having
arbitrary values, but only the values it has set itself (or the default
value). Notice that if the user initializes the AVCodecContext via
avcodec_parameters_from_context(), then it still counts as user-set even
though said function lives in libavcodec, too.

>> Imagine a scenario where the beginning (that gets
>> read
>> in avformat_find_stream_info()) of an input stream has e.g. embedded
>> cc
>> subtitles, but the part that lateron gets fed to the decoder does
>> not.
>> If the decoder has been initialized from the AVStream's
>> AVCodecParameters, then said decoder would still indicate that there
>> are
>> closed captions.* IIRC Anton already mentioned that this is actually
>> a
>> frame/packet property, not a stream property.
> 
> That statement was not thought through. Of course it IS a frame 
> property as well, but there's no difference to other properties like
> width, height or aspect ratio:
> 
> - as a frame property, it is indicating whether a frame carries 
>   closed caption data
> - as a stream property, it indicates the summary result from the 
>   probing process
> 
> Just the same like some other fields in this struct.
>

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Andreas Rheinhardt
> Sent: Wednesday, October 6, 2021 12:18 PM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v3 1/2] avcodec/codec_par: Add
> codec properties field to AVCodecParameters
> 
> Soft Works:
> >
> >
> >> -----Original Message-----
> >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> >> Andreas Rheinhardt
> >> Sent: Wednesday, October 6, 2021 11:30 AM
> >> To: ffmpeg-devel@ffmpeg.org
> >> Subject: Re: [FFmpeg-devel] [PATCH v3 1/2] avcodec/codec_par: Add
> >> codec properties field to AVCodecParameters
> >>
> >> Soft Works:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf
> Of
> >>>> Hendrik Leppkes
> >>>> Sent: Wednesday, October 6, 2021 8:57 AM
> >>>> To: FFmpeg development discussions and patches <ffmpeg-
> >>>> devel@ffmpeg.org>
> >>>> Subject: Re: [FFmpeg-devel] [PATCH v3 1/2] avcodec/codec_par:
> Add
> >>>> codec properties field to AVCodecParameters
> >>>>
> >>>> On Wed, Oct 6, 2021 at 8:45 AM Soft Works

[...]

> >>>
> >>
> >> The defines could be moved to defs.h (while just at it, we could
> also
> >> add properly prefixed alternatives and deprecate the FF_ ones).
> >> The copying that you are adding in
> avcodec_parameters_from_context
> >> is
> >> problematic, as the relevant AVCodecContext field is marked as
> "set
> >> by
> >> libavcodec" for decoding, which means that decoders expect it to
> be
> >> blank initially.
> >
> > Why do you think that decoders would expect it to be blank
> initially?
> >
> > What about the other fields then, like format, profile, level,
> width,
> > height, etc. - these are copied in the same way.
> > Are you saying that decoders expect all other fields to be filled
> but
> > the properties field to be zero?
> >
> 
> Of course not. E.g. the documentation of the dimension explicitly
> says
> so: "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."

The docs for properties don't say "May be set by the user".


> You completely misunderstood what my concern was about: In the
> scenario
> described above an API user querying the properties field would get
> the
> information that this stream contains CC data; even when it doesn't.

But where does that codec_par data come from?

When it's set by the user he has no reason to be surprised when 
it doesn't match reality.

When it comes from probing, then it DID have CC data during probing
which means that even if it the CC data has stopped to be transmitted
exactly within those few milliseconds between probing and decoding,
it is still likely that it will have CC data again at a future 
point in time.
(even though that's not a realistic scenario at all; practically
tv streams either have always cc or never cc)

Kind regards,
softworkz

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Andreas Rheinhardt
> Sent: Wednesday, October 6, 2021 12:18 PM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v3 1/2] avcodec/codec_par: Add
> codec properties field to AVCodecParameters
> 
> Soft Works:
> >
> >
> >> -----Original Message-----
> >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> >> Andreas Rheinhardt
> >> Sent: Wednesday, October 6, 2021 11:30 AM
> >> To: ffmpeg-devel@ffmpeg.org
> >> Subject: Re: [FFmpeg-devel] [PATCH v3 1/2] avcodec/codec_par: Add
> >> codec properties field to AVCodecParameters
> >>
> >> Soft Works:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf
> Of
> >>>> Hendrik Leppkes
> >>>> Sent: Wednesday, October 6, 2021 8:57 AM
> >>>> To: FFmpeg development discussions and patches <ffmpeg-
> >>>> devel@ffmpeg.org>
> >>>> Subject: Re: [FFmpeg-devel] [PATCH v3 1/2] avcodec/codec_par:
> Add
> >>>> codec properties field to AVCodecParameters
> >>>>
> >>>> On Wed, Oct 6, 2021 at 8:45 AM Soft Works
> <softworkz@hotmail.com>
> >>>> wrote:
> >>>>> diff --git a/libavcodec/codec_par.h b/libavcodec/codec_par.h
> >>>>> index 10cf79dff1..42ed8deb13 100644
> >>>>> --- a/libavcodec/codec_par.h
> >>>>> +++ b/libavcodec/codec_par.h
> >>>>> @@ -198,6 +198,10 @@ typedef struct AVCodecParameters {
> >>>>>       * Audio only. Number of samples to skip after a
> >>>> discontinuity.
> >>>>>       */
> >>>>>      int seek_preroll;
> >>>>> +    /**
> >>>>> +     * Codec properties of the stream that gets decoded
> >>>>> +     */
> >>>>> +    unsigned properties;
> >>>>>  } AVCodecParameters;
> >>>>>
> >>>>
> >>>> This field is severly underspecified/underdocumented. I realize
> >> you
> >>>> just copied it, but if I'm looking at this without pre-existing
> >>>> knowledge, then I have absolutely no idea what it might be for,
> or
> >>>> what kind of values go in there. The old field had the defines
> of
> >> the
> >>>> possible bits right there for context - this doesn't have this,
> so
> >> it
> >>>> would definitely benefit from added documentation.
> >>>
> >>> Hello Hendrik,
> >>>
> >>> that's right, but what would you suggest? We can't duplicate the
> >> defines,
> >>> though I could add a line in the help text like
> >>>
> >>>     /**
> >>>      * Codec properties of the stream that gets decoded.
> >>>      * Corresponds to @ref AVCodecContext.properties
> "properties".
> >>>      */
> >>>     unsigned properties;
> >>>
> >>> Would that be OK?
> >>>
> >>
> >> The defines could be moved to defs.h (while just at it, we could
> also
> >> add properly prefixed alternatives and deprecate the FF_ ones).
> >> The copying that you are adding in
> avcodec_parameters_from_context
> >> is
> >> problematic, as the relevant AVCodecContext field is marked as
> "set
> >> by
> >> libavcodec" for decoding, which means that decoders expect it to
> be
> >> blank initially.
> >
> > Why do you think that decoders would expect it to be blank
> initially?
> >
> > What about the other fields then, like format, profile, level,
> width,
> > height, etc. - these are copied in the same way.
> > Are you saying that decoders expect all other fields to be filled
> but
> > the properties field to be zero?
> >
> 
> Of course not. E.g. the documentation of the dimension explicitly
> says
> so: "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."
> The semantics of profile and level are closer, yet the difference is
> that if the decoder sets these fields, it overwrites any garbage it
> may
> have had; this is not so with a bitfield like properties.
> 
> > Also, the docs say that it's SET by decoders, not READ by decoders,
> > which means that no decoder could rely on this field. There's also
> > no undefined-state defined (like -1), which means that a decoder
> > couldn't know whether a 0-value indicates a valid value or a not-
> yet
> > initialized value.
> >
> 
> You completely misunderstood what my concern was about: In the
> scenario
> described above an API user querying the properties field would get
> the
> information that this stream contains CC data; even when it doesn't.
> 
> And your other claims are also wrong: A decoder may read anything
> from
> an AVCodecContext or from the AVPackets and AVFrames with the
> exception
> of data that is opaque to it. And if a field is described as
> out-of-touch by the user, then the codec may rely on it not having
> arbitrary values, but only the values it has set itself (or the
> default
> value). Notice that if the user initializes the AVCodecContext via
> avcodec_parameters_from_context(), then it still counts as user-set
> even
> though said function lives in libavcodec, too.

When you think that this is the wrong way, what would you suggest?

Thanks,
sw

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Soft Works
> Sent: Wednesday, October 6, 2021 2:29 PM
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH v3 1/2] avcodec/codec_par: Add
> codec properties field to AVCodecParameters
> 
> 
> 
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > Andreas Rheinhardt
> > Sent: Wednesday, October 6, 2021 12:18 PM
> > To: ffmpeg-devel@ffmpeg.org
> > Subject: Re: [FFmpeg-devel] [PATCH v3 1/2] avcodec/codec_par: Add
> > codec properties field to AVCodecParameters
> >
> > Soft Works:
> > >
> > >
> > >> -----Original Message-----
> > >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf
> Of
> > >> Andreas Rheinhardt
> > >> Sent: Wednesday, October 6, 2021 11:30 AM
> > >> To: ffmpeg-devel@ffmpeg.org
> > >> Subject: Re: [FFmpeg-devel] [PATCH v3 1/2] avcodec/codec_par:
> Add
> > >> codec properties field to AVCodecParameters
> > >>
> > >> Soft Works:
> > >>>
> > >>>
> > >>>> -----Original Message-----
> > >>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf
> > Of
> > >>>> Hendrik Leppkes
> > >>>> Sent: Wednesday, October 6, 2021 8:57 AM
> > >>>> To: FFmpeg development discussions and patches <ffmpeg-
> > >>>> devel@ffmpeg.org>
> > >>>> Subject: Re: [FFmpeg-devel] [PATCH v3 1/2] avcodec/codec_par:
> > Add
> > >>>> codec properties field to AVCodecParameters
> > >>>>
> > >>>> On Wed, Oct 6, 2021 at 8:45 AM Soft Works
> > <softworkz@hotmail.com>
> > >>>> wrote:
> > >>>>> diff --git a/libavcodec/codec_par.h b/libavcodec/codec_par.h
> > >>>>> index 10cf79dff1..42ed8deb13 100644
> > >>>>> --- a/libavcodec/codec_par.h
> > >>>>> +++ b/libavcodec/codec_par.h
> > >>>>> @@ -198,6 +198,10 @@ typedef struct AVCodecParameters {
> > >>>>>       * Audio only. Number of samples to skip after a
> > >>>> discontinuity.
> > >>>>>       */
> > >>>>>      int seek_preroll;
> > >>>>> +    /**
> > >>>>> +     * Codec properties of the stream that gets decoded
> > >>>>> +     */
> > >>>>> +    unsigned properties;
> > >>>>>  } AVCodecParameters;
> > >>>>>
> > >>>>
> > >>>> This field is severly underspecified/underdocumented. I
> realize
> > >> you
> > >>>> just copied it, but if I'm looking at this without pre-
> existing
> > >>>> knowledge, then I have absolutely no idea what it might be
> for,
> > or
> > >>>> what kind of values go in there. The old field had the defines
> > of
> > >> the
> > >>>> possible bits right there for context - this doesn't have
> this,
> > so
> > >> it
> > >>>> would definitely benefit from added documentation.
> > >>>
> > >>> Hello Hendrik,
> > >>>
> > >>> that's right, but what would you suggest? We can't duplicate
> the
> > >> defines,
> > >>> though I could add a line in the help text like
> > >>>
> > >>>     /**
> > >>>      * Codec properties of the stream that gets decoded.
> > >>>      * Corresponds to @ref AVCodecContext.properties
> > "properties".
> > >>>      */
> > >>>     unsigned properties;
> > >>>
> > >>> Would that be OK?
> > >>>
> > >>
> > >> The defines could be moved to defs.h (while just at it, we could
> > also
> > >> add properly prefixed alternatives and deprecate the FF_ ones).
> > >> The copying that you are adding in
> > avcodec_parameters_from_context
> > >> is
> > >> problematic, as the relevant AVCodecContext field is marked as
> > "set
> > >> by
> > >> libavcodec" for decoding, which means that decoders expect it to
> > be
> > >> blank initially.
> > >
> > > Why do you think that decoders would expect it to be blank
> > initially?
> > >
> > > What about the other fields then, like format, profile, level,
> > width,
> > > height, etc. - these are copied in the same way.
> > > Are you saying that decoders expect all other fields to be filled
> > but
> > > the properties field to be zero?
> > >
> >
> > Of course not. E.g. the documentation of the dimension explicitly
> > says
> > so: "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."
> > The semantics of profile and level are closer, yet the difference
> is
> > that if the decoder sets these fields, it overwrites any garbage it
> > may
> > have had; this is not so with a bitfield like properties.
> >
> > > Also, the docs say that it's SET by decoders, not READ by
> decoders,
> > > which means that no decoder could rely on this field. There's
> also
> > > no undefined-state defined (like -1), which means that a decoder
> > > couldn't know whether a 0-value indicates a valid value or a not-
> > yet
> > > initialized value.
> > >
> >
> > You completely misunderstood what my concern was about: In the
> > scenario
> > described above an API user querying the properties field would get
> > the
> > information that this stream contains CC data; even when it
> doesn't.
> >
> > And your other claims are also wrong: A decoder may read anything
> > from
> > an AVCodecContext or from the AVPackets and AVFrames with the
> > exception
> > of data that is opaque to it. And if a field is described as
> > out-of-touch by the user, then the codec may rely on it not having
> > arbitrary values, but only the values it has set itself (or the
> > default
> > value). Notice that if the user initializes the AVCodecContext via
> > avcodec_parameters_from_context(), then it still counts as user-set
> > even
> > though said function lives in libavcodec, too.
> 
> When you think that this is the wrong way, what would you suggest?

Hi Andreas,

even though I do not share the concern you mentioned, one way to address
this would be to include the properties field only when copying from
codec context to codec_par but not in the other direction.

Would that be better from your point of view?

Thanks,
softworkz