[FFmpeg-devel] [PATCH] lavf: Add an XMV demuxer

[Michael Niedermayer]

Hi Sven

On Tue, Aug 16, 2011 at 09:26:40PM +0200, Sven Hesse wrote:
> New version.
> 
> The wmv2dec hack was removed; the demuxer now manually converts the
> extradata into the standard WMV2 format. This has the added benefit
> that remuxing works now. Additionally, the keyframe flag is also
> passed to video packets containing key frames, this is apparently
> necessary for remuxing to work correctly.

This patch has been applied by diego and merged into
ffmpeg.

"late" Review below


[...]
> +
> +/**
> + * @file
> + * Microsoft XMV demuxer
> + */
> +
> +#include <stdint.h>
> +
> +#include "libavutil/intreadwrite.h"
> +
> +#include "avformat.h"
> +#include "riff.h"
> +
> +#define XMV_MIN_HEADER_SIZE 36
> +
> +#define XMV_AUDIO_ADPCM51_FRONTLEFTRIGHT 1
> +#define XMV_AUDIO_ADPCM51_FRONTCENTERLOW 2
> +#define XMV_AUDIO_ADPCM51_REARLEFTRIGHT  4
> +
> +#define XMV_AUDIO_ADPCM51 (XMV_AUDIO_ADPCM51_FRONTLEFTRIGHT | \
> +                           XMV_AUDIO_ADPCM51_FRONTCENTERLOW | \
> +                           XMV_AUDIO_ADPCM51_REARLEFTRIGHT)
> +
> +typedef struct XMVAudioTrack {
> +    uint16_t compression;
> +    uint16_t channels;
> +    uint32_t sample_rate;
> +    uint16_t bits_per_sample;
> +    uint32_t bit_rate;
> +    uint16_t flags;
> +    uint16_t block_align;
> +    uint16_t block_samples;
> +
> +    enum CodecID codec_id;
> +} XMVAudioTrack;
> +
> +typedef struct XMVVideoPacket {
> +    /* The decoder stream index for this video packet. */
> +    int stream_index;
> +
> +    uint32_t data_size;
> +    uint32_t data_offset;
> +
> +    uint32_t current_frame;
> +    uint32_t frame_count;
> +
> +    /* Does the video packet contain extra data? */
> +    int has_extradata;
> +
> +    /* Extra data */
> +    uint8_t extradata[4];
> +
> +    int64_t last_pts;
> +    int64_t pts;
> +} XMVVideoPacket;

The comments should use doxygen compatible syntax

The various offsets in the various structs should possibly be 64bit
or there may be need for integer overflow checks on their updates


> +
> +typedef struct XMVAudioPacket {
> +    /* The decoder stream index for this audio packet. */
> +    int stream_index;
> +
> +    /* The audio track this packet encodes. */
> +    XMVAudioTrack *track;
> +
> +    uint32_t data_size;
> +    uint32_t data_offset;
> +
> +    uint32_t frame_size;
> +
> +    uint32_t block_count;
> +} XMVAudioPacket;

this struct could be put in XMVAudioTrack to avoid the second malloc
& free, also it would possibly avoid a memleak if the second
alloc fails


[...]
> +    /** Initialize the packet context */
> +
> +    xmv->next_packet_offset = avio_tell(pb);
> +
> +    xmv->next_packet_size = this_packet_size - xmv->next_packet_offset;

> +    xmv->this_packet_size = 0;
> +
> +    xmv->video.current_frame = 0;
> +    xmv->video.frame_count   = 0;
> +    xmv->video.pts           = 0;
> +    xmv->video.last_pts      = 0;
> +
> +    xmv->current_stream = 0;

The struct fields should already be 0 at the begin from av_mallocz() thats
used to allocate the context


[...]
> +    /* Packet audio header */
> +
> +    for (audio_track = 0; audio_track < xmv->audio_track_count; audio_track++) {
> +        XMVAudioPacket *packet = &xmv->audio[audio_track];
> +
> +        if (avio_read(pb, data, 4) != 4)
> +            return AVERROR(EIO);
> +
> +        packet->data_size = AV_RL32(data) & 0x007FFFFF;
> +        if ((packet->data_size == 0) && (audio_track != 0))
> +            /* This happens when I create an XMV with several identical audio
> +             * streams. From the size calculations, duplicating the previous
> +             * stream's size works out, but the track data itself is silent.
> +             * Maybe this should also redirect the offset to the previous track?
> +             */
> +            packet->data_size = xmv->audio[audio_track - 1].data_size;
> +
> +        /** Carve up the audio data in frame_count slices */
> +        packet->frame_size  = packet->data_size  / xmv->video.frame_count;
> +        packet->frame_size -= packet->frame_size % packet->track->block_align;

Can these variables be 0 ? If so some checks are needed to avoid div
by zero


[...]
> +static int xmv_fetch_video_packet(AVFormatContext *s,
> +                                  AVPacket *pkt)
> +{
> +    XMVDemuxContext *xmv   = s->priv_data;
> +    AVIOContext     *pb    = s->pb;
> +    XMVVideoPacket  *video = &xmv->video;
> +
> +    int result;
> +    uint32_t frame_header;
> +    uint32_t frame_size, frame_timestamp;
> +    uint32_t i;
> +
> +    /* Seek to it */
> +    if (avio_seek(pb, video->data_offset, SEEK_SET) != video->data_offset)
> +        return AVERROR(EIO);
> +
> +    /* Read the frame header */
> +    frame_header = avio_rl32(pb);
> +
> +    frame_size      = (frame_header & 0x1FFFF) * 4 + 4;
> +    frame_timestamp = (frame_header >> 17);
> +
> +    if ((frame_size + 4) > video->data_size)
> +        return AVERROR(EIO);
> +
> +    /* Create the packet */
> +    result = av_new_packet(pkt, frame_size);
> +    if (result)
> +        return result;
> +
> +    /* Contrary to normal WMV2 video, the bit stream in XMV's
> +     * WMV2 is little-endian.
> +     * TODO: This manual swap is of course suboptimal.
> +     */
> +    for (i = 0; i < frame_size; i += 4)
> +        AV_WB32(pkt->data + i, avio_rl32(pb));

it might be faster (yet stil suboptimal) to read the whole and then
seperately bswap it
note there is also DSPContext.bswap_buf() but that may be hard/ugly
to use from a demuxer

Also you might want to add yourself to MAINTAINERS if you want to
maintain this demuxer in the future

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Into a blind darkness they enter who follow after the Ignorance,
they as if into a greater darkness enter who devote themselves
to the Knowledge alone. -- Isha Upanishad
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110818/bdf6b612/attachment.asc>