[FFmpeg-devel] [PATCH]pes packetizer

[Baptiste Coudurier]

Hi

realsun wrote:
> 
> [...]
> 
> --- pes.h	(revision 0)
> +++ pes.h	(revision 0)
> 
> [...]
>
> +#ifndef PES_H
> +#define PES_H

AVFORMAT_PES_H is better.

>
> [...]
>
> +
> +typedef struct {
> +    int is_ps;  /*< whether it is a Program Stream */
> +    int packet_number;
> +} PESContext;

We might have 3 possibilites (PS, TS, PES).
An enum is better IMHO.

>
> [...]
>
> + */
> +int ff_pes_mux_write(AVFormatContext *ctx, int stream_index,
> +          int64_t pts,int64_t dts, int  id, int startcode,
> +          uint8_t* pes_content, int pes_content_len,
> +          int header_len, int packet_size, int payload_size, int stuffing_size, int tailer_size);

Typo: tailer_size

>
> [...]
> 
> Property changes on: pes.h
> ___________________________________________________________________
> Name: svn:executable
>    + *

Executable ?

> 
> [...]
>
> --- mpegenc.c	(revision 9444)
> +++ mpegenc.c	(working copy)
>
> [...]
>
> +        if(s->is_mpeg2) {
> +            pes_context->packet_number = s->packet_number;
> +            pes_context->is_ps = 1;
> +            if (startcode == PRIVATE_STREAM_1) {
> +                bytestream_put_byte(&p, id);
> +                if (id >= 0xa0) {
> +                    /* LPCM (XXX: check nb_frames) */
> +                    bytestream_put_byte(&p, 7);
> +                    bytestream_put_be16(&p, 4); /* skip 3 header bytes */
>  
> -        nb_frames= get_nb_frames(ctx, stream, payload_size - stuffing_size);
> +                    bytestream_put_byte(&p, stream->lpcm_header[0]);
> +                    bytestream_put_byte(&p, stream->lpcm_header[1]);
> +                    bytestream_put_byte(&p, stream->lpcm_header[2]);
> +                } else if (id >= 0x40) {
> +                    /* AC3 */
> +                    nb_frames= ff_get_nb_frames(ctx, stream, payload_size - stuffing_size);
> +                    bytestream_put_byte(&p, nb_frames);
> +                    bytestream_put_be16(&p, trailer_size+1);
> +                }
> +            }

Current code handles PRIVATE_STREAM_1 even if !is_mpeg2, are you
changing muxer behaviour here ?

> +        } else {
>  
> -        put_be32(&ctx->pb, startcode);
> +            put_be32(&ctx->pb, startcode);
>  
> -        put_be16(&ctx->pb, packet_size);
> +            put_be16(&ctx->pb, packet_size);

Cosmetics (don't reindent code yet, reindent in next commit).

>
> [...]
>
> +            /* output data */
> +            if(av_fifo_generic_read(&pes_stream->fifo, payload_size - stuffing_size, &put_buffer, &ctx->pb) < 0) {
> +                return -1;
>              }
>          }
> -
> -        /* output data */
> -        if(av_fifo_generic_read(&stream->fifo, payload_size - stuffing_size, &put_buffer, &ctx->pb) < 0)
> -            return -1;

Again, don't reindent.

>
> [...]
>
> --- pesenc.c	(revision 9444)
> +++ pesenc.c	(working copy)
> 
> [...]
>
 > -    if (pad_packet_bytes > 0)
> -        put_padding_packet(ctx,&ctx->pb, pad_packet_bytes);
> +    /* special stuffing byte that is always written
> +       to prevent accidental generation of start codes. */
> +    put_byte(&ctx->pb, 0xff);
>  
> -    for(i=0;i<zero_trail_bytes;i++)
> -        put_byte(&ctx->pb, 0x00);
> +    for (i=0;i<stuffing_size;i++)
> +        put_byte(&ctx->pb, 0xff);
>  
> -    put_flush_packet(&ctx->pb);
> +     put_buffer(&ctx->pb, pes_content, pes_content_len);
>  

Can you try to not reindent the code and keep code in place, so we
clearly see what is removed and what is keep ?

>
> [...]
>
> +   /* output data */
> +    if(av_fifo_generic_read(&stream->fifo, data_size, &put_buffer, &ctx->pb) < 0)
> +        return -1;
> +    return data_size;

Isn't that duplicate of mpegenc.c ?

>
> [...]
>                  break;
>              }
>              stream->buffer_index -= pkt_desc->size;
> -
>              stream->predecode_packet= pkt_desc->next;
>              av_freep(&pkt_desc);
>          }
>      }
> -

Cosmectics.

>
> [...]
>
>      for(i=0; i<ctx->nb_streams; i++){
>          AVStream *st = ctx->streams[i];
> -        StreamInfo *stream = st->priv_data;
> +        PESStream*stream = st->priv_data;

Space between Stream and stream please.

>
> [...]
>  
> -    if(best_i < 0){
> +    if(*best_i < 0){
>          int64_t best_dts= INT64_MAX;
>  
>          for(i=0; i<ctx->nb_streams; i++){
>              AVStream *st = ctx->streams[i];
> -            StreamInfo *stream = st->priv_data;
> +            PESStream *stream = st->priv_data;
>              PacketDesc *pkt_desc= stream->predecode_packet;
>              if(pkt_desc && pkt_desc->dts < best_dts)
>                  best_dts= pkt_desc->dts;
> -        }
> +    }

Indentation problem ?

>
> [...]
>  
>  #if 0
>          av_log(ctx, AV_LOG_DEBUG, "bumping scr, scr:%f, dts:%f\n",
>                 scr/90000.0, best_dts/90000.0);
>  #endif
> +

Cosmetic.

[...]

-- 
Baptiste COUDURIER                              GnuPG Key Id: 0x5C1ABAAA
SMARTJOG S.A.                                    http://www.smartjog.com
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
Phone: +33 1 49966312