[Ffmpeg-devel] [PATCH] Add support for the NXV container

[Michael Niedermayer]

Hi

On Thu, Feb 22, 2007 at 11:47:19PM -0800, Eric Werness wrote:
> The NXV file format is used by the Brando MP4 watch and a few other 
> small gadgets that seem to be based on the same core.
> 
> The format is documented here: 
> http://wiki.multimedia.cx/index.php?title=NXV All of that is reverse 
> engineered, so it's likely that some parts have errors or are incomplete.
> 
> The decoder portion of this code has been tested against a dozen or so 
> NXV files encoded with the official Windows software at different 
> resolutions and quality levels.
> 
> The encoder currently only support one quality level, but with different 
> resolutions and framerates. The different quality levels, from a quick 
> look, appear to just be telling the watch to scale by 2x2 or 4x4 instead 
> of making the video a little window on the watch.
> 
> The A/V sync is a bit problematic, some of which is probably due to the 
> format relying on the MP3 bitrate staying stable and precise.

[...]
> +#include "avformat.h"
> +#include <float.h>
> +
> +#define AUDIO_PACKET_SIZE 500
> +#define INFO_SIZE 12
> +// Max frame size in bytes representable by the format
> +#define MAX_FRAME_SIZE (255*255*2)
> +// Number of frames buffered in the muxer
> +#define STAGING_FRAMES 3

the comments are not doxygen compatible
#define STAGING_FRAMES 3        ///< Number of frames buffered in the muxer
is what it should be


[...]

> +    vst->codec->sample_aspect_ratio = av_d2q(1.0, 10000);

(AVRational){1,1}


> +    // 500 bytes of 128kbit data is 32 packets/sec
> +    vst->r_frame_rate = av_d2q(32, 10000);

dont set this variable


> +    av_set_pts_info(vst, 33, 1, 90000);

this is wrong, set the correct timebase


[...]
> +    } else if (st->codec->codec_type == CODEC_TYPE_VIDEO) {
> +      if (st->codec->height > 255) {
> +	av_log(s, AV_LOG_ERROR, "height too large - try under 255\n");
> +	return -1;	    

tabs and trailing whitespace is forbidden in svn


[...]
> +      video_frame_rate = 1.0/av_q2d(st->codec->time_base);

float arithmetic is not allowed unless really required due to it breaking
regression tests besides it is inaccurate and leads to long term AV sync
issues


> +      
> +      ctx->width = st->codec->width;
> +      ctx->height = st->codec->height;

why duplicate the variables?


> +      ctx->frameSize = ctx->width * ctx->height * 2;
> +
> +      if (ctx->frameSize > MAX_FRAME_SIZE) {
> +	av_log(s, AV_LOG_ERROR, "Internal error: frame size greater than theoretical max\n");
> +	return -1;      
> +      }

this should be an assert() if its impossible, if not its not an internal error

[...]
> +  {
> +    // bits/sec / (bits/byte * bytes/buffer) = buffers/sec. Max frame rate is 1 buffer per frame
> +    double max_frame_rate = ((double)ctx->mp3_bit_rate)/(8.0*(double)AUDIO_PACKET_SIZE);
> +    double rate_ratio = max_frame_rate / video_frame_rate;
> +    double deviance = rate_ratio / round(rate_ratio);
> +    double recommended = (rate_ratio > 1.0) ? (max_frame_rate / round(rate_ratio)) : max_frame_rate;

as already said float arithmetic is not allowed

[...]
> +static inline int min(int a, int b) {return (a<b)?a:b;}

FFMIN()


> +
> +static int nxv_write_audio(AVFormatContext *s, NXVContext *ctx,
> +                           AVCodecContext *enc, const uint8_t *buf, int size)
> +{
> +  ByteIOContext *pb = &s->pb;
> +
> +  // Loop just in case we get >AUDIO_PACKET_SIZE bytes in a packet
> +  while (size) {
> +    int copySize = min(AUDIO_PACKET_SIZE - ctx->curAudioSize, size);
> +    
> +    // Fill the buffer if possible
> +    memcpy(ctx->audio+ctx->curAudioSize, buf, copySize);
> +    ctx->curAudioSize += copySize;
> +    buf += copySize;
> +    size -= copySize;
> +    
> +    // Flush an audio buffer if it's full
> +    assert(ctx->curAudioSize <= AUDIO_PACKET_SIZE);
> +    if (ctx->curAudioSize == AUDIO_PACKET_SIZE) {
> +      int i;
> +
> +      put_buffer(pb, ctx->audio, AUDIO_PACKET_SIZE);

maybe this could be done simpler with a AVFifoBuffer?

[...]
> +static int nxv_write_video(AVFormatContext *s, NXVContext *ctx,
> +                           AVCodecContext *enc, const uint8_t *buf, int size)
> +{
> +  int nextPut = (ctx->putFrame+1)%STAGING_FRAMES;
> +  uint8_t *out = ctx->video[nextPut];
> +  int i;
> +
> +  if (nextPut == ctx->getFrame) {
> +    av_log(s, AV_LOG_DEBUG, "Dropping a video frame to keep the streams in sync\n");

a muxer is not allowed to drop a frame, it MUST mux all or fatally fail
its the job of the user application (ffmpeg.c) to ensure that the framerate
is correct and acceptable
and its the job of AVOutputFormat.interleave_packet() to ensure correct
interleaving

[...]
> +static int nxv_read_packet(AVFormatContext *s,
> +                           AVPacket *pkt)
> +{
> +    NXVContext *ctx = s->priv_data;
> +    ByteIOContext *pb = &s->pb;
> +    uint8_t info[INFO_SIZE];
> +    uint32_t curSeq;
> +    int ret = 0;
> +
> +    if (ctx->mp3_bit_rate <= 0) {
> +      int i;
> +      for (i = 0; i < s->nb_streams; ++i) {
> +	AVStream *st = s->streams[i];
> +	if (st->codec->codec_type == CODEC_TYPE_AUDIO) {

as you created the streams in fixed order in nxv_read_header() they still
are in that order and hie loop is not needed


> +	  ctx->mp3_bit_rate = st->codec->bit_rate;
> +	  if (ctx->mp3_bit_rate > 0) {
> +	    // The pts increment corresponds to AUDIO_PACKET_SIZE bytes of audio at the specified bit rate
> +	    // bufRate = (bitrate/8) / AUDIO_PACKET_SIZE
> +	    // pts/90000 = buf#/bufRate = (AUDIO_PACKET_SIZE*buf#) / (bitrate/8)
> +	    ctx->frame_pts_inc = (90000 * AUDIO_PACKET_SIZE) / (ctx->mp3_bit_rate/8);

set the timebase correctly with av_set_pts_info() in nxv_read_header()


[...]
> +      for (i=0; i<ctx->expectingVideoBytes; i+=2) {
> +	uint8_t t = pkt->data[i]; pkt->data[i] = pkt->data[i+1]; pkt->data[i+1] = t;
> +      }

colorspace conversation in the demuxer is not allowed please add a pixfmt
to ../libavutil/avutil.h and dont forget little/big endian!
and conversation code to libswscale


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No human being will ever know the Truth, for even if they happen to say it
by chance, they would not even known they had done so. -- Xenophanes
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070223/a64fc804/attachment.pgp>