[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 at ffmpeg.org> On Behalf Of
> Andreas Rheinhardt
> Sent: Wednesday, October 6, 2021 11:30 AM
> To: ffmpeg-devel at 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 at ffmpeg.org> On Behalf Of
> >> Hendrik Leppkes
> >> Sent: Wednesday, October 6, 2021 8:57 AM
> >> To: FFmpeg development discussions and patches <ffmpeg-
> >> devel at 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 at 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