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

[zhentan feng]

Hi,

2008/3/31, Baptiste Coudurier <baptiste.coudurier at 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 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-31.patch
Type: application/octet-stream
Size: 12112 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-soc/attachments/20080331/569335f0/attachment.obj>