[FFmpeg-soc] [FFmpeg-devel] [Patch]GSoC 2008 qualification task TS Muxer

[Baptiste Coudurier]

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.

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