Tag: better-code

In Libav we try to clean up the API and make it more regular, this is one of the possibly many articles I write about APIs, this time about deprecating some relic from the past and why we are doing it.

AVPicture

This struct used to store image data using data pointers and linesizes. It comes from the far past and it looks like this:

typedef struct AVPicture {
    uint8_t *data[AV_NUM_DATA_POINTERS];
    int linesize[AV_NUM_DATA_POINTERS];
} AVPicture;

Once the AVFrame was introduced it was made so it would alias to it and for some time the two structures were actually defined sharing the commond initial fields through a macro.

The AVFrame then evolved to store both audio and image data, to use AVBuffer to not have to do needless copies and to provide more useful information (e.g. the actual data format), now it looks like:

typedef struct AVFrame {
    uint8_t *data[AV_NUM_DATA_POINTERS];
    int linesize[AV_NUM_DATA_POINTERS];

    uint8_t **extended_data;

    int width, height;

    int nb_samples;

    int format;

    int key_frame;

    enum AVPictureType pict_type;

    AVRational sample_aspect_ratio;

    int64_t pts;

    ...
} AVFrame;

The image-data manipulation functions moved to the av_image namespace and use directly data and linesize pointers, while the equivalent avpicture became a wrapper over them.

int avpicture_fill(AVPicture *picture, uint8_t *ptr,
                   enum AVPixelFormat pix_fmt, int width, int height)
{
    return av_image_fill_arrays(picture->data, picture->linesize,
                                ptr, pix_fmt, width, height, 1);
}

int avpicture_layout(const AVPicture* src, enum AVPixelFormat pix_fmt,
                     int width, int height,
                     unsigned char *dest, int dest_size)
{
    return av_image_copy_to_buffer(dest, dest_size,
                                   src->data, src->linesize,
                                   pix_fmt, width, height, 1);
}

...

It is also used in the subtitle abstraction:

typedef struct AVSubtitleRect {
    int x, y, w, h;
    int nb_colors;

    AVPicture pict;
    enum AVSubtitleType type;

    char *text;
    char *ass;
    int flags;
} AVSubtitleRect;

And to crudely pass AVFrame from the decoder level to the muxer level, for certain rawvideo muxers by doing something such as:

    pkt.data   = (uint8_t *)frame;
    pkt.size   =  sizeof(AVPicture);

AVPicture problems

In the codebase its remaining usage is dubious at best:

AVFrame as AVPicture

In some codecs the AVFrame produced or consumed are casted as AVPicture and passed to avpicture functions instead
of directly use the av_image functions.

AVSubtitleRect

For the subtitle codecs, accessing the Rect data requires a pointless indirection, usually something like:

    wrap3 = rect->pict.linesize[0];
    p = rect->pict.data[0];
    pal = (const uint32_t *)rect->pict.data[1];  /* Now in YCrCb! */

AVFMT_RAWPICTURE

Copying memory from a buffer to another when can be avoided is consider a major sin (“memcpy is murder”) since it is a costly operation in itself and usually it invalidates the cache if we are talking about large buffers.

Certain muxers for rawvideo, try to spare a memcpy and thus avoid a “murder” by not copying the AVFrame data to the AVPacket.

The idea in itself is simple enough, store the AVFrame pointer as if it would point a flat array, consider the data size as the AVPicture size and hope that the data pointed by the AVFrame remains valid while the AVPacket is consumed.

Simple and faulty: with the AVFrame ref-counted API codecs may use a Pool of AVFrames and reuse them.
It can lead to surprising results because the buffer gets updated before the AVPacket is actually written.
If the frame referenced changes dimensions or gets deallocated it could even lead to crashes.

Definitely not a great idea.

Solutions

Vittorio, wm4 and I worked together to fix the problems. Radically.

AVFrame as AVPicture

The av_image functions are now used when needed.
Some pointless copies got replaced by av_frame_ref, leading to less memory usage and simpler code.

No AVPictures are left in the video codecs.

AVSubtitle

The AVSubtitleRect is updated to have simple data and linesize fields and each codec is updated to keep the AVPicture and the new fields in sync during the deprecation window.

The code is already a little easier to follow now.

AVFMT_RAWPICTURE

Just dropping the “feature” would be a problem since those muxers are widely used in FATE and the time the additional copy takes adds up to quite a lot. Your regression test must be as quick as possible.

I wrote a safer wrapper pseudo-codec that leverages the fact that both AVPacket and AVFrame use a ref-counted system:

• The AVPacket takes the AVFrame and increases its ref-count by 1.
• The AVFrame is then stored in the data field and wrapped in a custom AVBuffer.
• That AVBuffer destructor callback unrefs the frame.

This way the AVFrame data won’t change until the AVPacket gets destroyed.

Goodbye AVPicture

With the release 14 the AVPicture struct will be removed completely from Libav, people using it outside Libav should consider moving to use full AVFrame (and leverage the additional feature it provides) or the av_image functions directly.