[Ffmpeg-devel] Re: [PATCH] GXF muxer
Baptiste Coudurier
baptiste.coudurier
Mon Jul 17 16:23:32 CEST 2006
Hi
Michael Niedermayer wrote:
> [...]
>>
>> +/* gxfenc.c */
>> +#ifdef CONFIG_GPL
>> +int gxfenc_init(void);
>> +#endif
>
> i think the prototype in the header doesnt need to be protected by that?
>
Ok.
> [...]
>> + 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
Changed.
> [...]
>> +enum PacketID {
>> + PKT_MAP = 0xBC,
>> + PKT_MEDIA = 0xBF,
>> + PKT_EOS = 0xFB,
>> + PKT_FLT,
>> + PKT_UMF
>> +};
>
> this should be shared between muxer and demuxer
It will simplified as soon as the muxer is ok.
> [...]
>
> code duplication, see ff_frame_rate_tab[]
>
I removed it and checked with picture resolution.
The order in gxf is different and Im afraid to break mpeg12.c
>> +};
>> +
>> +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
I will.
> [...]
>
> as the framerate table is the same as in mpeg12 this is a duplicate of
> the mpeg encoders frame rate selection code
>
Removed.
> [...]
>> +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()
>
Done.
> [...]
>> + put_le64(pb, av_gettime()); /* modification time */
>> + put_le64(pb, av_gettime()); /* creation time */
>
> use AVFormatContext.timestamp, this is unacceptable
>
Sorry, Done.
> [...]
>> + 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 ...
Well... It is ugly, but simple and clear. I don't really see how to
clean it.
> [...]
>> + 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
It could if the mpeg-2 stream would contain no I-Frame, is that possible ?
>> + 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
>
Well to ensure that it only use one char, cause of the header size
problem. GXF does not permit any padding packet or such concept.
>
> [...]
>> +
>> +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
Tried to.
>
>> + }
>> + memcpy(sc->audio_buffer_offset, pkt->data, pkt->size);
>
> this looks exploitable, theres absolutely no check for the size
>
Oops, sorry, removed.
>> + 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()
>
> [...]
>
Tried to, the code is duplicated from av_interleaved per dts, since I
need to reorder video frames. Audio is interleaved with the fifo. How
does it look like ?
Thanks again for your time, Michael.
--
Baptiste COUDURIER GnuPG Key Id: 0x5C1ABAAA
SMARTJOG S.A. http://www.smartjog.com
Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
Phone: +33 1 49966312
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: gxf_muxer.patch
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20060717/2c486d84/attachment.asc>
More information about the ffmpeg-devel
mailing list