[Ffmpeg-devel] [PATCH] GXF muxer
Michael Niedermayer
michaelni
Wed Jul 5 23:43:48 CEST 2006
Hi
On Wed, Jul 05, 2006 at 05:47:13PM +0200, Baptiste Coudurier wrote:
[...]
> Index: libavformat/allformats.h
> ===================================================================
> --- libavformat/allformats.h (revision 5624)
> +++ libavformat/allformats.h (working copy)
> @@ -144,6 +144,11 @@
> /* gxf.c */
> int gxf_init(void);
>
> +/* gxfenc.c */
> +#ifdef CONFIG_GPL
> +int gxfenc_init(void);
> +#endif
i think the prototype in the header doesnt need to be protected by that?
[...]
> + int i_per_gop;
> + int p_per_i;
> + int b_per_i;
the variables are missnamed as they count i,b,p frames idepandantly of the
per thing, only at the end will their meaning change
[...]
> +enum PacketID {
> + PKT_MAP = 0xBC,
> + PKT_MEDIA = 0xBF,
> + PKT_EOS = 0xFB,
> + PKT_FLT,
> + PKT_UMF
> +};
this should be shared between muxer and demuxer
[...]
> +static const AVRational gxf_frame_rate_tab[] = {
> + { 0, 0 },
> + { 1, 60 },
> + { 1001, 60000 },
> + { 1, 50 },
> + { 1, 30 },
> + { 1001, 30000 },
> + { 1, 25 },
> + { 1, 24 },
> + { 1001, 24000 },
> + { 0, 0 },
code duplication, see ff_frame_rate_tab[]
> +};
> +
> +static const CodecTag gxf_media_types[] = {
> + { CODEC_ID_MJPEG , 3 }, /* NTSC */
> + { CODEC_ID_MJPEG , 4 }, /* PAL */
> + { CODEC_ID_PCM_S24LE , 9 },
> + { CODEC_ID_PCM_S16LE , 10 },
> + { CODEC_ID_MPEG2VIDEO, 11 }, /* NTSC */
> + { CODEC_ID_MPEG2VIDEO, 12 }, /* PAL */
> + { CODEC_ID_DVVIDEO , 13 }, /* NTSC */
> + { CODEC_ID_DVVIDEO , 14 }, /* PAL */
> + { CODEC_ID_DVVIDEO , 15 }, /* 50M NTSC */
> + { CODEC_ID_DVVIDEO , 16 }, /* 50M PAL */
> + { CODEC_ID_AC3 , 17 },
> + //{ CODEC_ID_NONE, , 18 }, /* Non compressed 24 bit audio */
> + { CODEC_ID_MPEG2VIDEO, 20 }, /* MPEG HD */
> + { CODEC_ID_MPEG1VIDEO, 22 }, /* NTSC */
> + { CODEC_ID_MPEG1VIDEO, 23 }, /* PAL */
> + { 0, 0 },
> +};
this should be shared between muxer and demuxer
> +
> +#define SERVER_PATH "/space"
> +#define ES_NAME_PATTERN "ES."
> +
> +static int gxf_find_frame_rate_index(GXFStreamContext *ctx)
> +{
> + int i;
> + int64_t dmin = INT64_MAX;
> + int64_t d;
> +
> + for (i = 1; i < 9; i++) {
> + int64_t n0 = 1001LL / gxf_frame_rate_tab[i].num * gxf_frame_rate_tab[i].den * ctx->codec->time_base.num;
> + int64_t n1 = 1001LL * ctx->codec->time_base.den;
> + d = ABS(n0 - n1);
> + if (d < dmin) {
> + dmin = d;
> + ctx->frame_rate_index = i;
> + }
> + }
> + if (dmin)
> + return -1;
> + else
> + return 0;
> +}
as the framerate table is the same as in mpeg12 this is a duplicate of
the mpeg encoders frame rate selection code
[...]
> +static uint16_t gxf_codec_get_media_type(const CodecTag *tags, enum CodecID id)
> +{
> + while (tags->id != 0) {
> + if (id == tags->id)
> + return tags->tag;
> + tags++;
> + }
> + return 0;
> +}
use codec_get_tag()
[...]
> + put_le64(pb, av_gettime()); /* modification time */
> + put_le64(pb, av_gettime()); /* creation time */
use AVFormatContext.timestamp, this is unacceptable
[...]
> + switch (st->codec->codec_id) {
> + case CODEC_ID_MPEG1VIDEO:
> + sc->media_info = 'L' << 8;
> + sc->media_info |= mpeg1_tracks++;
> + break;
> + case CODEC_ID_MPEG2VIDEO:
> + sc->media_info = 'M' << 8;
> + sc->media_info |= mpeg2_tracks++;
> + break;
> + case CODEC_ID_PCM_S16LE:
> + sc->media_info = 'A' << 8;
> + sc->media_info |= audio_tracks++;
> + if (audio_tracks == ':')
> + audio_tracks = 'A'; /* first 10 audio tracks are 0 to 9 next 22 are A to V */
> + break;
> + case CODEC_ID_DVVIDEO:
> + sc->media_info = 'D' << 8;
> + sc->media_info |= dv_tracks++;
> + break;
> + case CODEC_ID_MJPEG:
> + sc->media_info = 'V' << 8;
> + sc->media_info |= '0';
> + break;
> + default:
> + sc->media_info = 'U' << 8;
> + sc->media_info |= '0';
> + }
the above can be implemented cleaner ...
[...]
> + if (sc->codec->codec_id == CODEC_ID_MPEG2VIDEO) {
> + int p = 0, b = 0;
> +
> + assert(sc->i_per_gop);
can this assert fail with some input, if so its unaccpetable
> + p = sc->p_per_i / sc->i_per_gop;
> + if (sc->p_per_i % sc->i_per_gop)
> + p++;
> + if (sc->p_per_i)
> + b = sc->b_per_i / sc->p_per_i;
now if i followed the missnamed variables correctly b is actually b_per_p ...
> + sc->p_per_i = p > 9 ? 9 : p;
> + sc->b_per_i = b > 9 ? 9 : b;
why 9
[...]
> +
> +static int gxf_write_packet(AVFormatContext *s, AVPacket *pkt)
> +{
> + GXFContext *gxf = s->priv_data;
> + GXFStreamContext *sc = &gxf->streams[pkt->stream_index];
> +
> + if (sc->codec->codec_type == CODEC_TYPE_AUDIO) {
> + if (sc->sample_count >= GXF_AUDIO_PACKET_SIZE) {
> + AVPacket npkt;
> +
> + av_new_packet(&npkt, GXF_AUDIO_PACKET_SIZE);
> + npkt.stream_index = pkt->stream_index;
> + memcpy(npkt.data, sc->audio_buffer, GXF_AUDIO_PACKET_SIZE);
> + gxf_write_media_packet(&s->pb, gxf, &npkt);
> + av_free_packet(&npkt);
> + sc->audio_buffer_offset -= GXF_AUDIO_PACKET_SIZE;
> + sc->sample_count -= GXF_AUDIO_PACKET_SIZE;
> + memmove(sc->audio_buffer, sc->audio_buffer + GXF_AUDIO_PACKET_SIZE, sc->sample_count);
> + gxf->audio_written++;
i belive a FifoBuffer could be used here
> + }
> + memcpy(sc->audio_buffer_offset, pkt->data, pkt->size);
this looks exploitable, theres absolutely no check for the size
> + sc->audio_buffer_offset += pkt->size;
> + sc->sample_count += pkt->size;
> + } else {
> + if (gxf->audio_written == gxf->audio_tracks) {
> + gxf_flush_video_packets(&s->pb, gxf, sc);
> + gxf->audio_written = 0;
> + }
> + av_new_packet(&sc->video_buffer_ptr->pkt, pkt->size);
> + memcpy(sc->video_buffer_ptr->pkt.data, pkt->data, pkt->size);
> + sc->video_buffer_ptr->pkt.flags = pkt->flags;
> + sc->video_buffer_ptr->next = av_malloc(sizeof(AVPacketList));
> + sc->video_buffer_ptr = sc->video_buffer_ptr->next;
> + }
i belive the whole can be simpified significantly by implementing
AVOutputFormat.interleave_packet()
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
In the past you could go to a library and read, borrow or copy any book
Today you'd get arrested for mere telling someone where the library is
More information about the ffmpeg-devel
mailing list