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

Soft Works softworkz at hotmail.com
Wed Oct 6 15:28:46 EEST 2021



> -----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?

Thanks,
sw


More information about the ffmpeg-devel mailing list