[Ffmpeg-devel] [PATCH] mtv demuxer genesis
Reynaldo H. Verdejo Pinochet
reynaldo
Mon Oct 9 23:39:17 CEST 2006
On Mon, Oct 09, 2006 at 12:15:03AM +0200, Michael Niedermayer wrote:
> Hi
> > + mtv->file_size = LE_32(&data[3]);
> > + mtv->segments = LE_32(&data[7]);
> > + mtv->audio_identifier = LE_24(&data[43]);
> > + mtv->audio_br=data[46];
> > + mtv->img_colorfmt = LE_24(&data[48]);
> > + mtv->img_bpp = data[51];
> > + mtv->img_width = data[52];
> > + mtv->img_height = data[54];
>
> the normal way to read stuff is get_le32() and its relatives unless
> theres some reason (complexity, size, ...) to do something else dunno
> if anything like that applies here ...
>
wasnt aware of that, fixed in the new version.
>
> > + /* all systems go! init decoders */
> > +
> > + /* video - raw rgb565 */
> > +
> > + st = av_new_stream(s, 0);
> > + if(!st)
> > + return AVERROR_NOMEM;
> > +
> > + av_set_pts_info(st, 64, 1, mtv->video_fps);
>
> maybe the following is more correct
> av_set_pts_info(st, 64, 4*data[62], data[46]);
> but either way the timebase must be accurate no rounding is allowed
> the above is just a guess based on how you set video_fps
>
i leaved this one as it was, i dont understand why its wrong/if.
>
> [...]
> > + // av_set_pts_info(st, 33, 1, 90000);s
>
> in absense of any other sane numbers samplerate is good choice
> for the timebase of audio streams ...
Ok, 44100 is the samplerate on all mtv's i have investigated.
>
>
> > + mtv->audio_stream_index = st->index;
>
> first stream created by av_new_stream() should be 0 second be 1 so these
> variables are redundant
>
Ok, fixed.
>
> [...]
> > + if(padding)
> > + {
> > + if(!(buffer = av_mallocz(padding)))
> > + return AVERROR_NOMEM;
> > +
> > + if(get_buffer(pb, buffer, padding) != padding)
> > + {
> > + av_free(buffer);
> > + return AVERROR_IO;
> > + }
> > +
> > + av_free(buffer);
>
> url_fskip()
Fixed
>
>
> > +
> > + if((ret = av_get_packet(pb, pkt, chunk_size - padding)) !=
> > + chunk_size - padding)
> > + return AVERROR_IO;
> > +
> > + pkt->stream_index = mtv->audio_stream_index;
> > +
> > + }else
> > + {
> > + buffer = av_mallocz(chunk_size);
>
> av_mallocz() does memset(0) the buffer which isnt needed here or?
>
Youre right, isnt needed, bad habit. Fixed
>
> > +
> > + if(!buffer)
> > + return AVERROR_NOMEM;
> > +
> > + if((ret = get_buffer(pb, buffer, chunk_size)) != chunk_size)
> > + {
> > + av_free(buffer);
> > + return AVERROR_IO;
> > + }
> > +
> > + if (av_new_packet(pkt, chunk_size))
> > + return AVERROR_IO;
>
> av_get_packet()
>
I do need av_new_packet here (AFAIK), see the updated version of the patch.
>
> > +
> > + /* buffer is GGGRRRR BBBBBGGG
> > + * and we need RRRRRGGG GGGBBBBB
> > + * for PIX_FMT_RGB565 so here we
> > + * just swap bytes as they come
> > + */
> > +
> > + for(i=0;i<chunk_size-1;i++)
> > + {
> > + tmp = *(buffer+i);
> > + *(buffer+i) = *(buffer+i+1);
> > + *(buffer+i+1) = tmp;
> > + }
>
> this is wrong one side is in ?(LE or BE) endianness as defined by the file
> format spec, the other side is cpu endianness and this should use bswap_16()
> and WORDS_BIGENDIAN
Already knew it was not endian safe, thanks for the WORDS_BIGENDIAN/bswap_16
tip, Im using them on the new version of the patch, hope its ok now.
>
>
> > +
> > + memcpy(pkt->data, buffer, chunk_size);
>
> can be avoided
>
yes, fixed.
>
> > + av_free(buffer);
> > + pkt->stream_index = mtv->video_stream_index;
> > + }
> > +
> > + return(ret);
> > +}
> > +
> > +static int mtv_read_close(AVFormatContext *s)
> > +{
> > + return 0;
> > +
> > +}
>
> if a function does nothing, setting it to NULL should work fine if not
> thats a bug
>
Removed then.
> [...]
Thanks
Hope to hear from you soon.
Reynaldo
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20061009/9e2dc8a2/attachment.pgp>
More information about the ffmpeg-devel
mailing list