[FFmpeg-devel] [Patch]GSoC 2008 qualification task TS Muxer
Michael Niedermayer
michaelni
Tue Mar 25 12:40:04 CET 2008
On Tue, Mar 25, 2008 at 11:57:16AM +0800, zhentan feng wrote:
> 2008/3/25, Michael Niedermayer <michaelni at gmx.at>:
[...]
> I modified the codes and attached 2 new patches.
>
> The patch names "TSMuxerPatch_svn_dev" is the 4 files against
> svn://svn.mplayerhq.hu/ffmpeg/trunk/libavformat
> I think if the patch works correctly, it would be our expect.
[...]
Review done based on diffs relative to the correct ancestor files.
> --- ffmpeg-svn/trunk/libavformat/mpegenc.c 2008-02-02 05:27:56.000000000 +0100
> +++ mpegpes.h 2008-03-25 12:00:15.000000000 +0100
[...]
> +/**
> + * PES packet description
> + */
> typedef struct PacketDesc {
> int64_t pts;
> int64_t dts;
> @@ -39,10 +42,13 @@
> struct PacketDesc *next;
> } PacketDesc;
>
> +/**
> + * PES stream structure
> + */
> typedef struct {
> AVFifoBuffer fifo;
> uint8_t id;
> - int max_buffer_size; /* in bytes */
> + int max_buffer_size; /**< in bytes */
> int buffer_index;
> PacketDesc *predecode_packet;
> PacketDesc *premux_packet;
These (and many other) changes are unrelated to factorizing the PES code out.
> --- ffmpeg-svn/trunk/libavformat/mpegenc.c 2008-02-02 05:27:56.000000000 +0100
> +++ mpegpesenc.c 2008-03-25 12:00:15.000000000 +0100
[...]
> switch(st->codec->codec_type) {
> case CODEC_TYPE_AUDIO:
> - if (st->codec->codec_id == CODEC_ID_AC3) {
> - stream->id = ac3_id++;
> - } else if (st->codec->codec_id == CODEC_ID_DTS) {
> - stream->id = dts_id++;
> - } else if (st->codec->codec_id == CODEC_ID_PCM_S16BE) {
> - stream->id = lpcm_id++;
> - for(j = 0; j < 4; j++) {
> - if (lpcm_freq_tab[j] == st->codec->sample_rate)
> - break;
> + if(ps_audio_bound != 0||ps_video_bound != 0){
> + if (st->codec->codec_id == CODEC_ID_AC3) {
> + stream->id = ac3_id++;
> + } else if (st->codec->codec_id == CODEC_ID_DTS) {
> + stream->id = dts_id++;
> + } else if (st->codec->codec_id == CODEC_ID_PCM_S16BE) {
> + stream->id = lpcm_id++;
> + for(j = 0; j < 4; j++) {
> + if (lpcm_freq_tab[j] == st->codec->sample_rate)
> + break;
All reindentions as well as other cosmetic changes must be in one or more
seperate patch(s)
[...]
> @@ -1171,7 +480,6 @@
> stream->next_packet= &pkt_desc->next;
>
> av_fifo_realloc(&stream->fifo, av_fifo_size(&stream->fifo) + size + 1);
> -
> if (s->is_dvd){
> if (is_iframe && (s->packet_number == 0 || (pts - stream->vobu_start_pts >= 36000))) { // min VOBU length 0.4 seconds (mpucoder)
> stream->bytes_to_iframe = av_fifo_size(&stream->fifo);
please remove all useless cosmetic changes
> Index: mpegenc.c
> ===================================================================
> --- mpegenc.c (revision 12559)
> +++ mpegenc.c (working copy)
[...]
> @@ -181,7 +157,7 @@
> int P_STD_max_mpeg_PS1 = 0;
>
> for(i=0;i<ctx->nb_streams;i++) {
> - StreamInfo *stream = ctx->streams[i]->priv_data;
> + PESStream *stream = ctx->streams[i]->priv_data;
Why is the struct renamed?
If there is some sense in this, then it must be in a seperate patch.
If there is no sense in this then it should not be renamed at all.
[...]
> @@ -314,82 +292,17 @@
>
> s->audio_bound = 0;
> s->video_bound = 0;
> - mpa_id = AUDIO_ID;
> - ac3_id = AC3_ID;
> - dts_id = DTS_ID;
> - mpv_id = VIDEO_ID;
> - mps_id = SUB_ID;
> - lpcm_id = LPCM_ID;
> - for(i=0;i<ctx->nb_streams;i++) {
> - st = ctx->streams[i];
> - stream = av_mallocz(sizeof(StreamInfo));
> - if (!stream)
> - goto fail;
> - st->priv_data = stream;
> +
trailing whitespace is forbidden in ffmpeg svn
> + if(ff_pes_muxer_init(ctx,ps_audio_bound,ps_video_bound) != 0)
> + goto fail;
>
[...]
> @@ -621,26 +534,12 @@
> put_byte(pb, 0xff);
> }
>
> -static int get_nb_frames(AVFormatContext *ctx, StreamInfo *stream, int len){
> - int nb_frames=0;
> - PacketDesc *pkt_desc= stream->premux_packet;
> -
> - while(len>0){
> - if(pkt_desc->size == pkt_desc->unwritten_size)
> - nb_frames++;
> - len -= pkt_desc->unwritten_size;
> - pkt_desc= pkt_desc->next;
> - }
> -
> - return nb_frames;
> -}
> -
> /* flush the packet on stream stream_index */
> static int flush_packet(AVFormatContext *ctx, int stream_index,
[...]
> if (packet_size > 0) {
> + int ps_flag = 1;
> + ff_pes_cal_header(ps_flag,s,id,stream,
> + &packet_size,&header_len,&pts,&dts,
> + &payload_size,&startcode,&stuffing_size,
> + &trailer_size,&pad_packet_bytes);
>
> - /* packet header size */
> - packet_size -= 6;
> + nb_frames= ff_pes_get_nb_frames(ctx, stream, payload_size - stuffing_size);
>
> - /* packet header */
> - if (s->is_mpeg2) {
> - header_len = 3;
> - if (stream->packet_number==0)
> - header_len += 3; /* PES extension */
[...]
>
> put_be16(ctx->pb, packet_size);
> @@ -987,105 +814,19 @@
> }
> #endif
>
> -static int remove_decoded_packets(AVFormatContext *ctx, int64_t scr){
> -// MpegMuxContext *s = ctx->priv_data;
> - int i;
> -
> - for(i=0; i<ctx->nb_streams; i++){
> - AVStream *st = ctx->streams[i];
> - StreamInfo *stream = st->priv_data;
> - PacketDesc *pkt_desc;
> -
> - while((pkt_desc= stream->predecode_packet)
> - && scr > pkt_desc->dts){ //FIXME > vs >=
> - if(stream->buffer_index < pkt_desc->size ||
> - stream->predecode_packet == stream->premux_packet){
> - av_log(ctx, AV_LOG_ERROR,
> - "buffer underflow i=%d bufi=%d size=%d\n",
> - i, stream->buffer_index, pkt_desc->size);
> - break;
> - }
> - stream->buffer_index -= pkt_desc->size;
> -
> - stream->predecode_packet= pkt_desc->next;
> - av_freep(&pkt_desc);
> - }
> - }
> -
> - return 0;
> -}
> -
> static int output_packet(AVFormatContext *ctx, int flush){
> MpegMuxContext *s = ctx->priv_data;
It would be very nice if you could split the different factorizations of
code into seperate patches.
[...]
> +#define AUDIO_ID 0xc0
> +#define VIDEO_ID 0xe0
> +#define AC3_ID 0x80
> +#define DTS_ID 0x8a
> +#define LPCM_ID 0xa0
> +#define SUB_ID 0x20
code duplication
[...]
>
> /* prepare packet header */
> q = buf;
> *q++ = 0x47;
> val = (ts_st->pid >> 8);
> - if (is_start)
> + if (is_start) {
> val |= 0x40;
> + is_start = 0;
> + }
> *q++ = val;
> *q++ = ts_st->pid;
> *q++ = 0x10 | ts_st->cc | (write_pcr ? 0x20 : 0);
> ts_st->cc = (ts_st->cc + 1) & 0xf;
> if (write_pcr) {
> + /* add header and pcr bytes to pcr according to specs */
> + pcr = ts->cur_pcr + (32+56) * 90000 / ts->mux_rate;
> *q++ = 7; /* AFC length */
> *q++ = 0x10; /* flags: PCR present */
> *q++ = pcr >> 25;
> @@ -524,59 +567,8 @@
> *q++ = pcr >> 1;
> *q++ = (pcr & 1) << 7;
> *q++ = 0;
> + ts->last_pcr = pcr;
> }
I assume these are bugfixes for the TS muxer? We certainly do want them but
they must be in a seperate patch. Not in the patch spliting the common PES
code out.
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Frequently ignored awnser#1 FFmpeg bugs should be sent to our bugtracker. User
questions about the command line tools should be sent to the ffmpeg-user ML.
And questions about how to use libav* should be sent to the libav-user ML.
-------------- 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/20080325/9cf0ecdc/attachment.pgp>
More information about the ffmpeg-devel
mailing list