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

Mark Thompson sw at jkqxz.net
Thu Sep 14 23:51:31 EEST 2017


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.

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

Yes; above there is:

"""
typedef struct CodedBitstreamUnit {
    /**
     * Codec-specific type of this unit.
     */
    uint32_t type;
"""

Thanks,

- Mark


More information about the ffmpeg-devel mailing list