Re: [FFmpeg-soc] [FFmpeg-devel] [Patch]GSoC 2008 qualification task TS Muxer
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~
Hi, zhentan feng wrote:
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 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.
Some are, and I fixed pcr computation.
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.
Yes, current TS muxer in soc is working, however mpegtsenc.c and mpegenc.c in soc svn shares too much code, you can see that already. Im not sure if code from ffmpeg svn mpegenc.c can be extracted right now, since It might not fit soc svn mpegtsenc.c code. First work on soc svn, and adapt mpegenc.c or mpegtsenc.c code if needed. See flush_packet for example, it contains much common code. You have to decide if it is worth to merge both functions and handle TS and PS cases, or just rewrite a very small flush_packet for TS. 'mpegpesenc.c' in soc svn should contain common functions, it already contains ff_pes_get_nb_frames for example, which is used by both muxer in soc svn.
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?
Yes. [...] -- Baptiste COUDURIER GnuPG Key Id: 0x5C1ABAAA SMARTJOG SAS http://www.smartjog.com Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA Phone: +33 1 49966312
2008/3/25, Baptiste Coudurier <baptiste.coudurier@smartjog.com>:
Hi,
zhentan feng wrote:
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 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.
Some are, and I fixed pcr computation.
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.
Yes, current TS muxer in soc is working, however mpegtsenc.c and mpegenc.c in soc svn shares too much code, you can see that already. Im not sure if code from ffmpeg svn mpegenc.c can be extracted right now, since It might not fit soc svn mpegtsenc.c code.
I have created a patch against svn soc and attached below again. This is just a draft to show that I think they could be extracted ,but it need to do some tricks (such as some NULL pointer, or flag)to make both mpegtsenc.c and mpegenc.c can share most of the codes extracted. So ,it maybe looks a little ugly :( If you agree the changes , I will make it work correctly as soon as possible:)
First work on soc svn, and adapt mpegenc.c or mpegtsenc.c code if needed. See flush_packet for example, it contains much common code. You have to decide if it is worth to merge both functions and handle TS and PS cases, or just rewrite a very small flush_packet for TS.
yes, I extract the common codes to ff_pes_cal_header() in mpegpesenc.c.
'mpegpesenc.c' in soc svn should contain common functions, it already contains ff_pes_get_nb_frames for example, which is used by both muxer in soc svn.
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?
Yes.
thanks for your reply, I'll try my best :)
[...]
--
Baptiste COUDURIER GnuPG Key Id: 0x5C1ABAAA SMARTJOG SAS http://www.smartjog.com Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA Phone: +33 1 49966312 _______________________________________________ FFmpeg-soc mailing list
FFmpeg-soc@mplayerhq.hu https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-soc
-- Best wishes~
2008/3/25, Baptiste Coudurier <baptiste.coudurier@smartjog.com>:
Hi,
zhentan feng wrote:
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 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.
Some are, and I fixed pcr computation.
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.
Yes, current TS muxer in soc is working, however mpegtsenc.c and mpegenc.c in soc svn shares too much code, you can see that already. Im not sure if code from ffmpeg svn mpegenc.c can be extracted right now, since It might not fit soc svn mpegtsenc.c code.
First work on soc svn, and adapt mpegenc.c or mpegtsenc.c code if needed. See flush_packet for example, it contains much common code. You have to decide if it is worth to merge both functions and handle TS and PS cases, or just rewrite a very small flush_packet for TS. 'mpegpesenc.c' in soc svn should contain common functions, it already contains ff_pes_get_nb_frames for example, which is used by both muxer in soc svn.
hi, I have modified the 4 files and copy them to currently svn-devel codes repo,overwrote mpegtsenc.c and mpegenc.c in /libavformat folder and add mpegpesenc.c ,mpegpes.h. Then I do "make" to complie the changed files and have fixed some errors. Now, all the 4 files can be complie successfully. The patch against svn-soc is attached below. I know the code from svn-soc is bugfixes. So,I want to know how to test my changes are right and do not break the function of the mpegtsenc.c and mpegenc.c against svn-soc? what's the sign of the codes can work correctly? Are there any sample files to play or some tools to judge it? Thanks for anyone could help me~
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?
Yes.
[...]
--
Baptiste COUDURIER GnuPG Key Id: 0x5C1ABAAA SMARTJOG SAS http://www.smartjog.com Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA Phone: +33 1 49966312 _______________________________________________ FFmpeg-soc mailing list
FFmpeg-soc@mplayerhq.hu https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-soc
-- Best wishes~
Hi, zhentan feng wrote:
2008/3/25, Baptiste Coudurier <baptiste.coudurier@smartjog.com>:
Hi,
zhentan feng wrote:
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 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.
Some are, and I fixed pcr computation.
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.
Yes, current TS muxer in soc is working, however mpegtsenc.c and mpegenc.c in soc svn shares too much code, you can see that already. Im not sure if code from ffmpeg svn mpegenc.c can be extracted right now, since It might not fit soc svn mpegtsenc.c code.
First work on soc svn, and adapt mpegenc.c or mpegtsenc.c code if needed. See flush_packet for example, it contains much common code. You have to decide if it is worth to merge both functions and handle TS and PS cases, or just rewrite a very small flush_packet for TS. 'mpegpesenc.c' in soc svn should contain common functions, it already contains ff_pes_get_nb_frames for example, which is used by both muxer in soc svn.
hi, I have modified the 4 files and copy them to currently svn-devel codes repo,overwrote mpegtsenc.c and mpegenc.c in /libavformat folder and add mpegpesenc.c ,mpegpes.h.
Yes, but please send seperate patches for every function ? This should make review easier, commit must be separate. Can you please attach your patches non base64-encoded ? It will be easier for me to review.
Then I do "make" to complie the changed files and have fixed some errors. Now, all the 4 files can be complie successfully. The patch against svn-soc is attached below.
I know the code from svn-soc is bugfixes. So,I want to know how to test my changes are right and do not break the function of the mpegtsenc.c and mpegenc.c against svn-soc?
You can know using FFmpeg svn, if new code is breaking regressions tests (make fulltest), then it's broken.
what's the sign of the codes can work correctly? Are there any sample files to play or some tools to judge it?
Not many that I know about unfortunately. PS muxer works great though. -- Baptiste COUDURIER GnuPG Key Id: 0x5C1ABAAA SMARTJOG SAS http://www.smartjog.com Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA Phone: +33 1 49966312
Hi,
zhentan feng wrote:
2008/3/25, Baptiste Coudurier <baptiste.coudurier@smartjog.com>:
Hi,
zhentan feng wrote:
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 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.
Some are, and I fixed pcr computation.
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.
Yes, current TS muxer in soc is working, however mpegtsenc.c and mpegenc.c in soc svn shares too much code, you can see that already. Im not sure if code from ffmpeg svn mpegenc.c can be extracted right now, since It might not fit soc svn mpegtsenc.c code.
First work on soc svn, and adapt mpegenc.c or mpegtsenc.c code if needed. See flush_packet for example, it contains much common code. You have to decide if it is worth to merge both functions and handle TS and PS cases, or just rewrite a very small flush_packet for TS. 'mpegpesenc.c' in soc svn should contain common functions, it already contains ff_pes_get_nb_frames for example, which is used by both muxer in soc svn.
hi, I have modified the 4 files and copy them to currently svn-devel codes repo,overwrote mpegtsenc.c and mpegenc.c in /libavformat folder and add mpegpesenc.c ,mpegpes.h.
Yes, but please send seperate patches for every function ? This should make review easier, commit must be separate. hi, I just review the codes and split the patch into 5 small patches manually according to their function. They are: 1,ff_pes_muxer_inti.patch to show the common codes in function ff_pes_muxer_inti(). 2,ff_pes_muxer_end.patch to show the common codes in function ff_pes_muxer_end(). 3,ff_pes_write_packet.patch to show the common codes in function ff_pes_write_packet(). 4,ff_pes_cal_header.patch to show the common codes in function ff_pes_cal_header(). 5,duplication_define_code.patch to show some redefination in mpegpes.h. 6,TSMuxer_soc_patch.patch is the integrity patch for checking usage. you can apply each patch every time to the base version,but do not apply two or more, it maybe confilict. I don't find an auoto-generated method to do this,so you add the 5
2008/3/26, Baptiste Coudurier <baptiste.coudurier@smartjog.com>: patches may not totally reflect the all changes ,so I attach the integrity patch named "TSMuxer_soc_patch.patch" to check.
Can you please attach your patches non base64-encoded ? It will be easier for me to review.
I am really sorry for this problem. I just use svn create the patches and send to the maillist by googlemail as attachment,I find all the mail setting optionals and search through the web but can't get how to use non base64-encoded. I will be very grateful,If you tell me what's the problem maybe.
Then I do "make" to complie the changed files and have fixed some errors. Now, all the 4 files can be complie successfully. The patch against svn-soc is attached below.
I know the code from svn-soc is bugfixes. So,I want to know how to test my changes are right and do not break the function of the mpegtsenc.c and mpegenc.c against svn-soc?
You can know using FFmpeg svn, if new code is breaking regressions tests (make fulltest), then it's broken.
About this ,I am still a little confused :( The situation is this: 1,In current svn_devel code repo, the mpegtsenc.c is broken, but I type the command "make test", then all is ok. 2,But the svn-soc you and your student last year has fixed the bug. So is it also will and *should* be ok,when I use the bugfixed codes to FFmpeg current source codes? i.e, are the regressions tests genarally applicability? Best wishes~
what's the sign of the codes can work correctly? Are there any sample files to play or some tools to judge it?
Not many that I know about unfortunately. PS muxer works great though.
--
Baptiste COUDURIER GnuPG Key Id: 0x5C1ABAAA SMARTJOG SAS http://www.smartjog.com Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA Phone: +33 1 49966312
-- Best wishes~
2008/3/27, zhentan feng <spyfeng@gmail.com>:
2008/3/26, Baptiste Coudurier <baptiste.coudurier@smartjog.com>:
Hi,
zhentan feng wrote:
2008/3/25, Baptiste Coudurier <baptiste.coudurier@smartjog.com>:
Hi,
zhentan feng wrote:
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 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.
Some are, and I fixed pcr computation.
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.
Yes, current TS muxer in soc is working, however mpegtsenc.c and mpegenc.c in soc svn shares too much code, you can see that already. Im not sure if code from ffmpeg svn mpegenc.c can be extracted right now, since It might not fit soc svn mpegtsenc.c code.
First work on soc svn, and adapt mpegenc.c or mpegtsenc.c code if needed. See flush_packet for example, it contains much common code. You have to decide if it is worth to merge both functions and handle TS and PS cases, or just rewrite a very small flush_packet for TS. 'mpegpesenc.c' in soc svn should contain common functions, it already contains ff_pes_get_nb_frames for example, which is used by both muxer in soc svn.
hi, I have modified the 4 files and copy them to currently svn-devel codes repo,overwrote mpegtsenc.c and mpegenc.c in /libavformat folder and add mpegpesenc.c ,mpegpes.h.
Yes, but please send seperate patches for every function ? This should make review easier, commit must be separate.
hi, I just review the codes and split the patch into 5 small patches manually according to their function. They are: 1,ff_pes_muxer_inti.patch to show the common codes in function ff_pes_muxer_inti(). 2,ff_pes_muxer_end.patch to show the common codes in function ff_pes_muxer_end(). 3,ff_pes_write_packet.patch to show the common codes in function ff_pes_write_packet(). 4,ff_pes_cal_header.patch to show the common codes in function ff_pes_cal_header(). 5,duplication_define_code.patch to show some redefination in mpegpes.h. 6,TSMuxer_soc_patch.patch is the integrity patch for checking usage. you can apply each patch every time to the base version,but do not apply two or more, it maybe confilict. I don't find an auoto-generated method to do this,so you add the 5 patches may not totally reflect the all changes ,so I attach the integrity patch named "TSMuxer_soc_patch.patch" to check.
Today,I have fixed some bugs in mpegpesenc.c. I attached 3 new patches as below: 1,ff_pes_muxer_init_new.patch to show the common codes in function ff_pes_muxer_init(). 2,ff_pes_cal_header.patch to show the common codes in funtion ff_pes_cal_header(). 3,TSMuxer_soc_Newpactch.patch to show all the changes against svn soc codes repo. you can just apply the TSMuxer_soc_Newpatch.patch to svn-soc codes get the current code files. I run the 4files in svn-devel codes repo and "make full test" and got the same results with running the svn-soc files in svn-devel codes repo. So, I conservativly think the changes do not break the regressions tests. Thank you very much to review the patches.
Can you please attach your patches non base64-encoded ? It will be easier for me to review.
I am really sorry for this problem. I just use svn create the patches and send to the maillist by googlemail as attachment,I find all the mail setting optionals and search through the web but can't get how to use non base64-encoded. I will be very grateful,If you tell me what's the problem maybe.
Then I do "make" to complie the changed files and have fixed some errors. Now, all the 4 files can be complie successfully. The patch against svn-soc is attached below.
I know the code from svn-soc is bugfixes. So,I want to know how to test my changes are right and do not break the function of the mpegtsenc.c and mpegenc.c against svn-soc?
You can know using FFmpeg svn, if new code is breaking regressions tests (make fulltest), then it's broken.
About this ,I am still a little confused :( The situation is this: 1,In current svn_devel code repo, the mpegtsenc.c is broken, but I type the command "make test", then all is ok. 2,But the svn-soc you and your student last year has fixed the bug. So is it also will and *should* be ok,when I use the bugfixed codes to FFmpeg current source codes? i.e, are the regressions tests genarally applicability?
Best wishes~
what's the sign of the codes can work correctly? Are there any sample files to play or some tools to judge it?
Not many that I know about unfortunately. PS muxer works great though.
--
Baptiste COUDURIER GnuPG Key Id: 0x5C1ABAAA SMARTJOG SAS http://www.smartjog.com Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA Phone: +33 1 49966312
-- Best wishes~
-- Best wishes~
Could you please stop cross-posting and keep this discussion on ffmpeg-soc? Diego
2008/3/28, Diego Biurrun <diego@biurrun.de>:
Could you please stop cross-posting and keep this discussion on ffmpeg-soc?
i am sorry and i will follow the rule. Thank you
Diego
_______________________________________________ FFmpeg-soc mailing list FFmpeg-soc@mplayerhq.hu https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-soc
-- Best wishes~
Hi, On Thu, Mar 27, 2008 at 10:52:09PM +0800, zhentan feng wrote:
Index: mpegenc.c =================================================================== --- mpegenc.c (revision 2048) +++ mpegenc.c (working copy) @@ -266,11 +266,13 @@ static int mpeg_mux_init(AVFormatContext *ctx) { MpegMuxContext *s = ctx->priv_data; - int bitrate, i, mpa_id, mpv_id, mps_id, ac3_id, dts_id, lpcm_id, j; + int bitrate, i; AVStream *st; PESStream *stream; int audio_bitrate; int video_bitrate; + int *ps_audio_bound = &(s->audio_bound); + int *ps_video_bound = &(s->video_bound);
[...]
+ if(ff_pes_muxer_init(ctx,ps_audio_bound,ps_video_bound) != 0)
Code seems to only count audio and video streams, I don't think ff_pes_muxer_init needs to be extented, only count them in mpeg_mux_init (a small for loop should do the trick).
[...]
/** + * Caculate the PES header to flush + * @param[in] ps_flag the sign for PS, '1' is PS, '0' is TS + * @param[in] is_mpeg2 the pointer point to PS struct + * @param[in] is_dvd the pointer point to PS struct
[...]
+ */ +void ff_pes_cal_header(int ps_flag,int *is_mpeg2,int *is_dvd,int id,PESStream *stream, + int *packet_size,int *header_len,int64_t *pts,int64_t *dts, + int *payload_size,int *startcode,int *stuffing_size, + int *trailer_size,int *pad_packet_bytes); + +/**
I think some flags should be used for different variants: #define FMT_MPEG2 0x01 #define FMT_VCD 0x02 | FMT_MPEG2 #define FMT_SVCD 0x04 | FMT_MPEG2 #define FMT_DVD 0x08 | FMT_MPEG2 #define FMT_TS 0x10 | FMT_MPEG2 then add a "format" field in PESStream. You will be able to check with (s->format & FMT_MPEG2). Beware of mpeg1system muxer though. It should be simpler and cleaner, and will avoid passing 3 args. This needs a separate patch. [...] -- Baptiste COUDURIER GnuPG Key Id: 0x5C1ABAAA SMARTJOG SAS http://www.smartjog.com Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA Phone: +33 1 49966312
2008/3/28, Baptiste Coudurier <baptiste.coudurier@smartjog.com>:
Hi,
On Thu, Mar 27, 2008 at 10:52:09PM +0800, zhentan feng wrote:
Index: mpegenc.c ===================================================================
--- mpegenc.c (revision 2048) +++ mpegenc.c (working copy) @@ -266,11 +266,13 @@ static int mpeg_mux_init(AVFormatContext *ctx) {
MpegMuxContext *s = ctx->priv_data;
- int bitrate, i, mpa_id, mpv_id, mps_id, ac3_id, dts_id, lpcm_id, j; + int bitrate, i; AVStream *st; PESStream *stream; int audio_bitrate; int video_bitrate; + int *ps_audio_bound = &(s->audio_bound); + int *ps_video_bound = &(s->video_bound);
[...]
+ if(ff_pes_muxer_init(ctx,ps_audio_bound,ps_video_bound) != 0)
Code seems to only count audio and video streams, I don't think ff_pes_muxer_init needs to be extented, only count them in mpeg_mux_init (a small for loop should do the trick).
yes. ps_audio_bound, and ps_video_bound are just flags to keep off TS can not execute some codes when call the function ff_pes_muxer_init(). I review the codes again , find mepg_mux_init() in mpegenc.c and mpegts_write_header() in mpegtsenc.c actually have small common codes. Moreover, mpegtsenc.c initial the stream data as below: for(i=0;i<ctx->nb_streams;i++) { st = ctx->streams[i]; stream = st->priv_data; but mpegenc.c initial the stream like this: 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; Thus, I think it is stiff resued without flags to sign PS or TS stream. So, I leave it just as svn-soc original.
[...]
/** + * Caculate the PES header to flush + * @param[in] ps_flag the sign for PS, '1' is PS, '0' is TS + * @param[in] is_mpeg2 the pointer point to PS struct + * @param[in] is_dvd the pointer point to PS struct
[...]
+ */ +void ff_pes_cal_header(int ps_flag,int *is_mpeg2,int *is_dvd,int id,PESStream *stream, + int *packet_size,int *header_len,int64_t *pts,int64_t *dts, + int *payload_size,int *startcode,int *stuffing_size, + int *trailer_size,int *pad_packet_bytes); + +/**
I think some flags should be used for different variants:
#define FMT_MPEG2 0x01 #define FMT_VCD 0x02 | FMT_MPEG2 #define FMT_SVCD 0x04 | FMT_MPEG2 #define FMT_DVD 0x08 | FMT_MPEG2 #define FMT_TS 0x10 | FMT_MPEG2
then add a "format" field in PESStream.
You will be able to check with (s->format & FMT_MPEG2).
Beware of mpeg1system muxer though.
It should be simpler and cleaner, and will avoid passing 3 args.
This needs a separate patch.
The patch names "ff_pes_cal_header_3-29.patch" show the changes.
[...]
-- Baptiste COUDURIER GnuPG Key Id: 0x5C1ABAAA SMARTJOG SAS http://www.smartjog.com Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA Phone: +33 1 49966312
_______________________________________________ FFmpeg-soc mailing list FFmpeg-soc@mplayerhq.hu https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-soc
-- Best wishes~
Hi, On Sun, Mar 30, 2008 at 02:28:35AM +0800, zhentan feng wrote:
2008/3/28, Baptiste Coudurier <baptiste.coudurier@smartjog.com>:
Hi,
On Thu, Mar 27, 2008 at 10:52:09PM +0800, zhentan feng wrote:
Index: mpegenc.c ===================================================================
--- mpegenc.c (revision 2048) +++ mpegenc.c (working copy) @@ -266,11 +266,13 @@ static int mpeg_mux_init(AVFormatContext *ctx) {
MpegMuxContext *s = ctx->priv_data;
- int bitrate, i, mpa_id, mpv_id, mps_id, ac3_id, dts_id, lpcm_id, j; + int bitrate, i; AVStream *st; PESStream *stream; int audio_bitrate; int video_bitrate; + int *ps_audio_bound = &(s->audio_bound); + int *ps_video_bound = &(s->video_bound);
[...]
+ if(ff_pes_muxer_init(ctx,ps_audio_bound,ps_video_bound) != 0)
Code seems to only count audio and video streams, I don't think ff_pes_muxer_init needs to be extented, only count them in mpeg_mux_init (a small for loop should do the trick).
yes. ps_audio_bound, and ps_video_bound are just flags to keep off TS can not execute some codes when call the function ff_pes_muxer_init(). I review the codes again , find mepg_mux_init() in mpegenc.c and mpegts_write_header() in mpegtsenc.c actually have small common codes.
Well, you can use !(stream->format & PES_FMT_TS)
Moreover, mpegtsenc.c initial the stream data as below: for(i=0;i<ctx->nb_streams;i++) { st = ctx->streams[i]; stream = st->priv_data;
but mpegenc.c initial the stream like this: 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;
Thus, I think it is stiff resued without flags to sign PS or TS stream. So, I leave it just as svn-soc original.
Uniformizing code is always welcome if you think it is worth, but as always clean and seperate patches.
[...]
+ */ +void ff_pes_cal_header(int ps_flag,int *is_mpeg2,int *is_dvd,int id,PESStream *stream, + int *packet_size,int *header_len,int64_t *pts,int64_t *dts, + int *payload_size,int *startcode,int *stuffing_size, + int *trailer_size,int *pad_packet_bytes); + +/**
I think some flags should be used for different variants:
#define FMT_MPEG2 0x01 #define FMT_VCD 0x02 | FMT_MPEG2 #define FMT_SVCD 0x04 | FMT_MPEG2 #define FMT_DVD 0x08 | FMT_MPEG2 #define FMT_TS 0x10 | FMT_MPEG2
then add a "format" field in PESStream.
You will be able to check with (s->format & FMT_MPEG2).
Beware of mpeg1system muxer though.
It should be simpler and cleaner, and will avoid passing 3 args.
This needs a separate patch.
The patch names "ff_pes_cal_header_3-29.patch" show the changes.
[...]
+ /*just consider dvd and mpeg2 for ff_pes_cal_header()*/ + if(s->is_mpeg2){ + stream->format = FMT_MPEG2; + if(s->is_dvd) + stream->format = FMT_DVD | FMT_MPEG2; + } id = stream->id;
You must get rid of is_mpeg and is_dvd if you have FMT_* Btw, after rethinking, PES_FMT_* is more appropriate. Furthermore, the new mechanism can be added separately, it is non functionnal. So this means the patch must be separate.
[...]
Index: mpegpes.h =================================================================== --- mpegpes.h (revision 2048) +++ mpegpes.h (working copy) @@ -30,6 +30,12 @@ #include "avformat.h" #include "fifo.h"
+#define FMT_MPEG2 0x01 +#define FMT_VCD 0x02 +#define FMT_SVCD 0x04 +#define FMT_DVD 0x08 +#define FMT_TS 0x10 +
SVCD/DVD/TS are MPEG2 so you can add FMT_MPEG2 to constants. This will avoid some checks.
[...]
+ /* packet header */ + if (!(stream->format & FMT_TS)) { + *header_len = 3; + *header_len += 1; /* obligatory stuffing byte */ + }else if ((stream->format) & FMT_MPEG2){ + *header_len = 3; + if (stream->packet_number==0) + *header_len += 3; /* PES extension */ + *header_len += 1; /* obligatory stuffing byte */
Indentation looks weird here.
[...] + // first byte does not fit -> reset pts/dts + stuffing + if(*payload_size <= *trailer_size && (*pts) != AV_NOPTS_VALUE){ + int timestamp_len=0; + if(*dts != *pts) + timestamp_len += 5; + if(*pts != AV_NOPTS_VALUE){ + if(!(stream->format & FMT_TS)) + timestamp_len += stream->format & FMT_MPEG2 ? 5 : 4; + else + timestamp_len += 5;
This can be simplified, TS is MPEG2. Anyway, first send the patch using the new mechanism alone, you will see that this will simplify much the few cases which you have to escape for TS. And beware of warnings and useless parenthesis, I saw a few. [...] -- Baptiste COUDURIER GnuPG Key Id: 0x5C1ABAAA SMARTJOG SAS http://www.smartjog.com Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA Phone: +33 1 49966312
Hi, 2008/3/30, Baptiste Coudurier <baptiste.coudurier@smartjog.com>:
Hi,
On Sun, Mar 30, 2008 at 02:28:35AM +0800, zhentan feng wrote:
2008/3/28, Baptiste Coudurier <baptiste.coudurier@smartjog.com>:
Hi,
On Thu, Mar 27, 2008 at 10:52:09PM +0800, zhentan feng wrote:
Index: mpegenc.c ===================================================================
--- mpegenc.c (revision 2048) +++ mpegenc.c (working copy) @@ -266,11 +266,13 @@ static int mpeg_mux_init(AVFormatContext *ctx) {
MpegMuxContext *s = ctx->priv_data;
- int bitrate, i, mpa_id, mpv_id, mps_id, ac3_id, dts_id, lpcm_id, j; + int bitrate, i; AVStream *st; PESStream *stream; int audio_bitrate; int video_bitrate; + int *ps_audio_bound = &(s->audio_bound); + int *ps_video_bound = &(s->video_bound);
[...]
+ if(ff_pes_muxer_init(ctx,ps_audio_bound,ps_video_bound) != 0)
Code seems to only count audio and video streams, I don't think ff_pes_muxer_init needs to be extented, only count them in mpeg_mux_init (a small for loop should do the trick).
yes. ps_audio_bound, and ps_video_bound are just flags to keep off TS can not execute some codes when call the function ff_pes_muxer_init(). I review the codes again , find mepg_mux_init() in mpegenc.c and mpegts_write_header() in mpegtsenc.c actually have small common codes.
Well, you can use !(stream->format & PES_FMT_TS)
Moreover, mpegtsenc.c initial the stream data as below: for(i=0;i<ctx->nb_streams;i++) { st = ctx->streams[i]; stream = st->priv_data;
but mpegenc.c initial the stream like this: 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;
Thus, I think it is stiff resued without flags to sign PS or TS stream. So, I leave it just as svn-soc original.
Uniformizing code is always welcome if you think it is worth, but as always clean and seperate patches.
[...]
+ */ +void ff_pes_cal_header(int ps_flag,int *is_mpeg2,int *is_dvd,int id,PESStream *stream, + int *packet_size,int *header_len,int64_t *pts,int64_t *dts, + int *payload_size,int *startcode,int *stuffing_size, + int *trailer_size,int *pad_packet_bytes); + +/**
I think some flags should be used for different variants:
#define FMT_MPEG2 0x01 #define FMT_VCD 0x02 | FMT_MPEG2 #define FMT_SVCD 0x04 | FMT_MPEG2 #define FMT_DVD 0x08 | FMT_MPEG2 #define FMT_TS 0x10 | FMT_MPEG2
then add a "format" field in PESStream.
You will be able to check with (s->format & FMT_MPEG2).
Beware of mpeg1system muxer though.
It should be simpler and cleaner, and will avoid passing 3 args.
This needs a separate patch.
The patch names "ff_pes_cal_header_3-29.patch" show the changes.
[...]
+ /*just consider dvd and mpeg2 for ff_pes_cal_header()*/ + if(s->is_mpeg2){ + stream->format = FMT_MPEG2; + if(s->is_dvd) + stream->format = FMT_DVD | FMT_MPEG2; + } id = stream->id;
You must get rid of is_mpeg and is_dvd if you have FMT_* Btw, after rethinking, PES_FMT_* is more appropriate.
well, I remove the assignments to "stream->format" to the function mpeg_mux_init(). you can find it in the new patch. Since is_dvd and is_mpeg2 appear many times in mpeg_mux_init(), at the same time,PESStream haven't been initialized, so I think we cannot get rid of is_dvd and is_mpeg2.Moreover, they are variables in MpegMuxContext and may be used in somewhere else.
Furthermore, the new mechanism can be added separately, it is non functionnal. So this means the patch must be separate.
[...]
Index: mpegpes.h =================================================================== --- mpegpes.h (revision 2048) +++ mpegpes.h (working copy) @@ -30,6 +30,12 @@ #include "avformat.h" #include "fifo.h"
+#define FMT_MPEG2 0x01 +#define FMT_VCD 0x02 +#define FMT_SVCD 0x04 +#define FMT_DVD 0x08 +#define FMT_TS 0x10 +
SVCD/DVD/TS are MPEG2 so you can add FMT_MPEG2 to constants.
This will avoid some checks.
fixed.
[...]
+ /* packet header */ + if (!(stream->format & FMT_TS)) { + *header_len = 3; + *header_len += 1; /* obligatory stuffing byte */ + }else if ((stream->format) & FMT_MPEG2){ + *header_len = 3;
+ if (stream->packet_number==0) + *header_len += 3; /* PES extension */
+ *header_len += 1; /* obligatory stuffing byte */
Indentation looks weird here.
:P Actually,the logic seems confusing and I have fixed it.
[...] + // first byte does not fit -> reset pts/dts + stuffing + if(*payload_size <= *trailer_size && (*pts) != AV_NOPTS_VALUE){ + int timestamp_len=0; + if(*dts != *pts) + timestamp_len += 5; + if(*pts != AV_NOPTS_VALUE){ + if(!(stream->format & FMT_TS)) + timestamp_len += stream->format & FMT_MPEG2 ? 5 : 4; + else + timestamp_len += 5;
This can be simplified, TS is MPEG2.
Anyway, first send the patch using the new mechanism alone, you will see that this will simplify much the few cases which you have to escape for TS.
And beware of warnings and useless parenthesis, I saw a few.
yes. Thank you very much to point them out. The new patch names "ff_pes_cal_header_3-30.patch" as below.
[...]
-- Baptiste COUDURIER GnuPG Key Id: 0x5C1ABAAA SMARTJOG SAS http://www.smartjog.com Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA Phone: +33 1 49966312 _______________________________________________ FFmpeg-soc mailing list FFmpeg-soc@mplayerhq.hu https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-soc
-- Best wishes~
On Mon, Mar 31, 2008 at 12:48:00AM +0800, zhentan feng wrote:
[...]
+ /*just consider dvd and mpeg2 for ff_pes_cal_header()*/ + if(s->is_mpeg2){ + stream->format = FMT_MPEG2; + if(s->is_dvd) + stream->format = FMT_DVD | FMT_MPEG2; + } id = stream->id;
You must get rid of is_mpeg and is_dvd if you have FMT_* Btw, after rethinking, PES_FMT_* is more appropriate.
well, I remove the assignments to "stream->format" to the function mpeg_mux_init(). you can find it in the new patch. Since is_dvd and is_mpeg2 appear many times in mpeg_mux_init(), at the same time,PESStream haven't been initialized, so I think we cannot get rid of is_dvd and is_mpeg2.Moreover, they are variables in MpegMuxContext and may be used in somewhere else.
Having a format field in MpexMuxContext and in StreamInfo, is not that bad IMHO, still it merges 5 fields into one, but yes, this could be done separately.
Furthermore, the new mechanism can be added separately, it is non functionnal. So this means the patch must be separate.
[...]
Index: mpegpes.h =================================================================== --- mpegpes.h (revision 2048) +++ mpegpes.h (working copy) @@ -30,6 +30,12 @@ #include "avformat.h" #include "fifo.h"
+#define FMT_MPEG2 0x01 +#define FMT_VCD 0x02 +#define FMT_SVCD 0x04 +#define FMT_DVD 0x08 +#define FMT_TS 0x10 +
SVCD/DVD/TS are MPEG2 so you can add FMT_MPEG2 to constants.
This will avoid some checks.
fixed.
Well, why don't you always define TS as MPEG2 ?
[...]
+void ff_pes_cal_header(int id,PESStream * stream, + int *packet_size,int *header_len,int64_t *pts,int64_t *dts, + int *payload_size,int *startcode,int *stuffing_size, + int *trailer_size,int *pad_packet_bytes) +{ + /* packet header size */ + *packet_size -= 6;
+ /* packet header */ + if (stream->format & PES_FMT_TS) { + *header_len = 3; + *header_len += 1; /* obligatory stuffing byte */ + }else if (stream->format & PES_FMT_MPEG2){ + *header_len = 3; + if (stream->packet_number==0) + *header_len += 3; /* PES extension */ + *header_len += 1; /* obligatory stuffing byte */ + } else { + *header_len = 0; + }
This can be merged in & PES_FMT_MPEG2 case.
[...]
+ *pts=*dts=AV_NOPTS_VALUE; + *header_len -= timestamp_len; + if ((stream->format & PES_FMT_DVD) && stream->align_iframe){ + *pad_packet_bytes += timestamp_len; + *packet_size -= timestamp_len; + } + else { + *payload_size += timestamp_len; + } + *stuffing_size += timestamp_len; + if(*payload_size > *trailer_size) + *stuffing_size += *payload_size - *trailer_size; + }
Indentation looks weird.
[...]
int pes_size; uint8_t* q = stream->payload; + pes_stream->format = PES_FMT_TS|PES_FMT_MPEG2;
See top comment. [...] -- Baptiste COUDURIER GnuPG Key Id: 0x5C1ABAAA SMARTJOG SAS http://www.smartjog.com Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA Phone: +33 1 49966312
Hi, 2008/3/31, Baptiste Coudurier <baptiste.coudurier@smartjog.com>:
On Mon, Mar 31, 2008 at 12:48:00AM +0800, zhentan feng wrote:
[...]
+ /*just consider dvd and mpeg2 for ff_pes_cal_header()*/ + if(s->is_mpeg2){ + stream->format = FMT_MPEG2; + if(s->is_dvd) + stream->format = FMT_DVD | FMT_MPEG2; + } id = stream->id;
You must get rid of is_mpeg and is_dvd if you have FMT_* Btw, after rethinking, PES_FMT_* is more appropriate.
well, I remove the assignments to "stream->format" to the function mpeg_mux_init(). you can find it in the new patch. Since is_dvd and is_mpeg2 appear many times in mpeg_mux_init(), at the same time,PESStream haven't been initialized, so I think we cannot get rid of is_dvd and is_mpeg2.Moreover, they are variables in MpegMuxContext and may be used in somewhere else.
Having a format field in MpexMuxContext and in StreamInfo, is not that bad IMHO, still it merges 5 fields into one, but yes, this could be done separately.
That's a good idea,I will considerate it later and finish it separately.
Furthermore, the new mechanism can be added separately, it is non functionnal. So this means the patch must be separate.
[...]
Index: mpegpes.h =================================================================== --- mpegpes.h (revision 2048) +++ mpegpes.h (working copy) @@ -30,6 +30,12 @@ #include "avformat.h" #include "fifo.h"
+#define FMT_MPEG2 0x01 +#define FMT_VCD 0x02 +#define FMT_SVCD 0x04 +#define FMT_DVD 0x08 +#define FMT_TS 0x10 +
SVCD/DVD/TS are MPEG2 so you can add FMT_MPEG2 to constants.
This will avoid some checks.
fixed.
Well, why don't you always define TS as MPEG2 ?
I just think it is easy to check the format just use if (s->format & PES_FMT_*), but it seems not reflect the relationship with eachother. you are right, I have changed them.
[...]
+void ff_pes_cal_header(int id,PESStream * stream,
+ int *packet_size,int *header_len,int64_t *pts,int64_t *dts, + int *payload_size,int *startcode,int *stuffing_size, + int *trailer_size,int *pad_packet_bytes)
+{
+ /* packet header size */ + *packet_size -= 6;
+ /* packet header */ + if (stream->format & PES_FMT_TS) {
+ *header_len = 3; + *header_len += 1; /* obligatory stuffing byte */
+ }else if (stream->format & PES_FMT_MPEG2){
+ *header_len = 3; + if (stream->packet_number==0) + *header_len += 3; /* PES extension */ + *header_len += 1; /* obligatory stuffing byte */
+ } else { + *header_len = 0; + }
This can be merged in & PES_FMT_MPEG2 case.
fixed
[...]
+ *pts=*dts=AV_NOPTS_VALUE; + *header_len -= timestamp_len; + if ((stream->format & PES_FMT_DVD) && stream->align_iframe){ + *pad_packet_bytes += timestamp_len; + *packet_size -= timestamp_len; + } + else { + *payload_size += timestamp_len; + } + *stuffing_size += timestamp_len; + if(*payload_size > *trailer_size) + *stuffing_size += *payload_size - *trailer_size; + }
Indentation looks weird.
fixed
[...]
int pes_size; uint8_t* q = stream->payload; + pes_stream->format = PES_FMT_TS|PES_FMT_MPEG2;
See top comment.
fixed The new patch names "ff_pes_cal_header_3-31.patch" attached below. Thank you.
[...]
-- Baptiste COUDURIER GnuPG Key Id: 0x5C1ABAAA SMARTJOG SAS http://www.smartjog.com Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA Phone: +33 1 49966312 _______________________________________________ FFmpeg-soc mailing list FFmpeg-soc@mplayerhq.hu https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-soc
-- Best wishes~
Hi, On Mon, Mar 31, 2008 at 11:18:07PM +0800, zhentan feng wrote:
[...]
Index: mpegpes.h =================================================================== --- mpegpes.h (revision 2048) +++ mpegpes.h (working copy) @@ -30,6 +30,12 @@ #include "avformat.h" #include "fifo.h"
+#define PES_FMT_MPEG2 0x01 +#define PES_FMT_VCD 0x02 +#define PES_FMT_SVCD 0x04 | PES_FMT_MPEG2 +#define PES_FMT_DVD 0x08 | PES_FMT_MPEG2 +#define PES_FMT_TS 0x10 | PES_FMT_MPEG2 +
Yes, but problem now is that == PES_FMT_TS is causing problems, and gcc warns you about it. So you should directly merge values I guess.
[...]
Index: mpegpesenc.c =================================================================== --- mpegpesenc.c (revision 2048) +++ mpegpesenc.c (working copy) @@ -100,7 +100,109 @@
return nb_frames; } +/** + * Caculate the PES header to flush + * @param[in] id the stream id + * @param[in] stream the PES stream + * @param[in] packet_size the packet size for PES + * @param[in] header_len the PES header length + * @param[in] pts the PTS + * @param[in] dts the DTS + * @param[in] payload_size the PES palyload size + * @param[in] startcode the startcode + * @param[in]stuffing_size the PES stuff size + * @param[in] trailer_size the trailer_ size + * @param[in] pad_packet_bytes the padding size for packet + * @return NULL + */ +void ff_pes_cal_header(int id,PESStream * stream, + int *packet_size,int *header_len,int64_t *pts,int64_t *dts, + int *payload_size,int *startcode,int *stuffing_size, + int *trailer_size,int *pad_packet_bytes) +{ + /* packet header size */ + *packet_size -= 6;
+ /* packet header */ + if (stream->format & PES_FMT_MPEG2){ + *header_len = 3; + if (stream->packet_number==0) + *header_len += 3; /* PES extension */
You don't check for TS here ? Old code is not checking this. Are you sure it is correct ? Did you test it ? Except that patch is ok. -- Baptiste COUDURIER GnuPG Key Id: 0x5C1ABAAA SMARTJOG SAS http://www.smartjog.com Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA Phone: +33 1 49966312
2008/4/1, Baptiste Coudurier <baptiste.coudurier@smartjog.com>:
Hi,
On Mon, Mar 31, 2008 at 11:18:07PM +0800, zhentan feng wrote:
[...]
Index: mpegpes.h =================================================================== --- mpegpes.h (revision 2048) +++ mpegpes.h (working copy) @@ -30,6 +30,12 @@ #include "avformat.h" #include "fifo.h"
+#define PES_FMT_MPEG2 0x01 +#define PES_FMT_VCD 0x02 +#define PES_FMT_SVCD 0x04 | PES_FMT_MPEG2 +#define PES_FMT_DVD 0x08 | PES_FMT_MPEG2 +#define PES_FMT_TS 0x10 | PES_FMT_MPEG2 +
Yes, but problem now is that == PES_FMT_TS is causing problems, and gcc warns you about it. So you should directly merge values I guess.
fixed. The warnings are caused by the operator priority.
[...]
Index: mpegpesenc.c =================================================================== --- mpegpesenc.c (revision 2048) +++ mpegpesenc.c (working copy) @@ -100,7 +100,109 @@
return nb_frames; } +/**
+ * Caculate the PES header to flush
+ * @param[in] id the stream id + * @param[in] stream the PES stream + * @param[in] packet_size the packet size for PES + * @param[in] header_len the PES header length + * @param[in] pts the PTS + * @param[in] dts the DTS + * @param[in] payload_size the PES palyload size + * @param[in] startcode the startcode + * @param[in]stuffing_size the PES stuff size + * @param[in] trailer_size the trailer_ size + * @param[in] pad_packet_bytes the padding size for packet + * @return NULL + */
+void ff_pes_cal_header(int id,PESStream * stream, + int *packet_size,int *header_len,int64_t *pts,int64_t *dts, + int *payload_size,int *startcode,int *stuffing_size, + int *trailer_size,int *pad_packet_bytes) +{ + /* packet header size */ + *packet_size -= 6;
+ /* packet header */
+ if (stream->format & PES_FMT_MPEG2){ + *header_len = 3; + if (stream->packet_number==0) + *header_len += 3; /* PES extension */
You don't check for TS here ? Old code is not checking this. Are you sure it is correct ? Did you test it ?
yes, I think it will be strict to check the packet_number in TS. Actually , the packet_number is useless for TS. if( packet_number == 0 ) just the test for PS writing the PES extension for P-STD_buffer_flag and TS don't necessary write the 3 bytes extension flag. The new patch names "ff_pes_cal_header_4-1.patch" attached as below. thank you
Except that patch is ok.
-- Baptiste COUDURIER GnuPG Key Id: 0x5C1ABAAA SMARTJOG SAS http://www.smartjog.com Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA Phone: +33 1 49966312 _______________________________________________ FFmpeg-soc mailing list FFmpeg-soc@mplayerhq.hu https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-soc
-- Best wishes~
participants (3)
-
Baptiste Coudurier -
Diego Biurrun -
zhentan feng