[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
> Soft Works
> Sent: Wednesday, October 6, 2021 2:29 PM
> 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
> 
> 
> 
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> > Andreas Rheinhardt
> > Sent: Wednesday, October 6, 2021 12:18 PM
> > 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
> > >> 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?
> > >
> >
> > 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