[FFmpeg-devel] [PATCH 03/20] lavc: Add coded bitstream read/write API

[Michael Niedermayer]

On Thu, Sep 14, 2017 at 09:51:31PM +0100, Mark Thompson wrote:
> On 14/09/17 21:28, Michael Niedermayer wrote:
> > On Thu, Sep 14, 2017 at 08:31:28AM +0100, Mark Thompson wrote:
> >> On 14/09/17 01:42, Michael Niedermayer wrote:
> >>> On Wed, Sep 13, 2017 at 12:43:53AM +0100, Mark Thompson wrote:
> > [...]
> > 
> >>>> +int ff_cbs_write_packet(CodedBitstreamContext *ctx,
> >>>> +                        AVPacket *pkt,
> >>>> +                        CodedBitstreamFragment *frag)
> >>>> +{
> >>>> +    int err;
> >>>> +
> >>>> +    err = ff_cbs_write_fragment_data(ctx, frag);
> >>>> +    if (err < 0)
> >>>> +        return err;
> >>>> +
> >>>
> >>>> +    av_new_packet(pkt, frag->data_size);
> >>>> +    if (err < 0)
> >>>> +        return err;
> >>>
> >>> that does not work
> >>
> >> I think I'm missing something.  Can you be more precise about what doesn't work?
> > 
> > you ignore the return code of av_new_packet()
> > the check does nothing
> 
> Duh, oops.  I spent a while looking for some subtlety in av_new_packet() and missed the obvious.  Sorry!
> 
> >>>> +     */
> >>>> +    int              nb_units;
> >>>> +    /**
> >>>> +     * Pointer to an array of units of length nb_units.
> >>>> +     *
> >>>> +     * Must be NULL if nb_units is zero.
> >>>> +     */
> >>>> +    CodedBitstreamUnit *units;
> >>>> +} CodedBitstreamFragment;
> >>>> +
> >>>> +/**
> >>>> + * Context structure for coded bitstream operations.
> >>>> + */
> >>>> +typedef struct CodedBitstreamContext {
> >>>> +    /**
> >>>> +     * Logging context to be passed to all av_log() calls associated
> >>>> +     * with this context.
> >>>> +     */
> >>>> +    void *log_ctx;
> >>>> +
> >>>> +    /**
> >>>> +     * Internal codec-specific hooks.
> >>>> +     */
> >>>> +    const struct CodedBitstreamType *codec;
> >>>> +
> >>>> +    /**
> >>>> +     * Internal codec-specific data.
> >>>> +     *
> >>>> +     * This contains any information needed when reading/writing
> >>>> +     * bitsteams which will not necessarily be present in a fragment.
> >>>> +     * For example, for H.264 it contains all currently visible
> >>>> +     * parameter sets - they are required to determine the bitstream
> >>>> +     * syntax but need not be present in every access unit.
> >>>> +     */
> >>>> +    void *priv_data;
> >>>> +
> >>>
> >>>> +    /**
> >>>> +     * Array of unit types which should be decomposed when reading.
> >>>> +     *
> >>>> +     * Types not in this list will be available in bitstream form only.
> >>>> +     * If NULL, all supported types will be decomposed.
> >>>> +     */
> >>>> +    uint32_t *decompose_unit_types;
> >>>
> >>> this should not be a generic integer.
> >>> a typedef or enum would be better
> >>
> >> It's H.264 nal unit types union H.265 nal unit types union MPEG-2 start codes (union any other codec that gets added).
> >>
> >> What type should that be?  I chose uint32_t to make it fixed-size and hopefully large enough to be able to make sense for any future codec.
> > 
> > a typedef based type would be better than a litterally hardcoded
> > one. A hardcoded type would be duplicatedly hardcoded in many
> > places ...
> 
> So have "typedef uint32_t CBSUnitType;"?  I'm not really sure this adds anything since every implementation is using its own enum anyway, but ok.

duplication is bad
consider you ever want to change the underlaying type ...

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I have often repented speaking, but never of holding my tongue.
-- Xenocrates
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170914/3314540a/attachment.sig>