2008/3/25, Michael Niedermayer <michaelni@gmx.at>:
On Tue, Mar 25, 2008 at 11:57:16AM +0800, zhentan feng wrote:
2008/3/25, Michael Niedermayer <michaelni@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.
Thanks. I have fixed it.
--- 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)
I see :)
[...]
@@ -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
fixed
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
fixed
+ 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.
I see :)
[...]
+#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
I will fix it.
[...]
/* 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.
yes, these are bugfixes.But it was modified by the student last year,not me. The situation is that I just download the codes from svn://svn.mplayerhq.hu/soc/dvbmuxer and I got 4 files mpegtsenc.c, mpegenc.c,mpegpesenc.c,mpegpes.h,which are already bug fixed for mpegtsenc.c against the svn-devl codes, and already extracted some common codes. According to baptiste.coudurie wrote in the early mail,I have just extracted more common codes and do not do any functional changes. As qualification task, I think I will do the next 2 things: 1.Test my codes , confirm it works correctly. 2.Create different patches for different situation: i) create patches against svn://svn.mplayerhq.hu/soc/dvbmuxer to show my changes from the last year's students. ii) create patches against svn-devel codes to show the bug fixed and common codes extracted from mpegtsenc.c and mpegenc.c to mpegpesenc.c. am I right? thank you very much.
[...]
-- 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.
-----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.6 (GNU/Linux)
iD8DBQFH6OSSYR7HhwQLD6sRAk+9AKCTQiebtxysgiwlCJI/MGRj8yeW8QCdHzmN iPdnWr7Y+hhjjF3vz8K8sGU= =1wM7 -----END PGP SIGNATURE-----
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@mplayerhq.hu https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
-- Best wishes~