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

[Baptiste Coudurier]

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