[FFmpeg-soc] [FFmpeg-devel] [Patch]GSoC 2008 qualification task TS Muxer
zhentan feng
spyfeng at gmail.com
Sun Mar 30 18:48:00 CEST 2008
Hi,
2008/3/30, Baptiste Coudurier <baptiste.coudurier at smartjog.com>:
> Hi,
>
>
> On Sun, Mar 30, 2008 at 02:28:35AM +0800, zhentan feng wrote:
> > 2008/3/28, Baptiste Coudurier <baptiste.coudurier at 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 at mplayerhq.hu
> https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-soc
>
--
Best wishes~
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ff_pes_cal_header_3-30.patch
Type: application/octet-stream
Size: 12249 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-soc/attachments/20080331/63dd5e1f/attachment.obj>
More information about the FFmpeg-soc
mailing list