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

[Michael Niedermayer]

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


[...]


> >> +     */
> >> +    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 ...

Also please document that the type is codec specific (it is IIUC)

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The misfortune of the wise is better than the prosperity of the fool.
-- Epicurus
-------------- 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/4d7e4257/attachment.sig>