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

Michael Niedermayer michaelni
Tue Mar 25 12:40:04 CET 2008


On Tue, Mar 25, 2008 at 11:57:16AM +0800, zhentan feng wrote:
> 2008/3/25, Michael Niedermayer <michaelni at 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.


> --- 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)


[...]

> @@ -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




> 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


> +    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.


[...]

> +#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


[...]

>  
>          /* 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.


[...]
-- 
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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080325/9cf0ecdc/attachment.pgp>



More information about the ffmpeg-devel mailing list