[FFmpeg-devel] [PATCH]pes packetizer

[Michael Niedermayer]

On Fri, Aug 31, 2007 at 05:43:35PM +0800, Xiaohui Sun wrote:
[...]
>>
>> [...]
>>
>>   
>>> +    put_byte(&ctx->pb, 0x80); /* mpeg2 id */
>>>  -        if (!s->is_mpeg2)
>>> -            for(i=0;i<stuffing_size;i++)
>>> -                put_byte(&ctx->pb, 0xff);
>>> -
>>> -        if (s->is_mpeg2) {
>>> -            put_byte(&ctx->pb, 0x80); /* mpeg2 id */
>>>     
>>
>> as already said in past reviews, this is a cosmetic change and does not
>> belong in here, please do not reindent the code
>>   
> fixed

no, its not fixed:

> +    put_byte(&ctx->pb, 0x80); /* mpeg2 id */
>  
> -        if (!s->is_mpeg2)
> -            for(i=0;i<stuffing_size;i++)
> -                put_byte(&ctx->pb, 0xff);
> -
> -        if (s->is_mpeg2) {
> -            put_byte(&ctx->pb, 0x80); /* mpeg2 id */


[...]

>>>  }
>>>  -static void put_vcd_padding_sector(AVFormatContext *ctx)
>>> +int ff_pes_remove_decoded_packets(AVFormatContext *ctx, int64_t scr)
>>>     
>> [...]
>>   
>>> -static int remove_decoded_packets(AVFormatContext *ctx, int64_t scr){
>>>     
>>
>> as ive said in past reviews
>> please put this function rename in a seperate patch
>>   
> these two function has no relation, and I don't know why they were diffed 
> against each other...

these fuctions are identical

--- old	2007-08-31 20:13:30.000000000 +0200
+++ new	2007-08-31 20:13:42.000000000 +0200
@@ -1,10 +1,10 @@
-static int remove_decoded_packets(AVFormatContext *ctx, int64_t scr){
-//    MpegMuxContext *s = ctx->priv_data;
+int ff_pes_remove_decoded_packets(AVFormatContext *ctx, int64_t scr)
+{
     int i;
 
     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;
 
         while((pkt_desc= stream->predecode_packet)


[...]
>  }
>  
> -static inline void put_timestamp(ByteIOContext *pb, int id, int64_t timestamp)
> +inline void put_timestamp(ByteIOContext *pb, int id, int64_t timestamp)
>  {

this lacks a ff_ prefix now, which would make the code after this patch
broken so we cannot apply it, svn must always be working
commiting broken code with the intent to correct it at some unspecified
point in the future is not good
its better not to break it in the first place


[...]
>  typedef struct {
> -    AVFifoBuffer fifo;
> +    PESStream pes_stream;
>      uint8_t id;
> -    int max_buffer_size; /* in bytes */
> -    int buffer_index;
> -    PacketDesc *predecode_packet;
> -    PacketDesc *premux_packet;
> -    PacketDesc **next_packet;
>      int packet_number;
>      uint8_t lpcm_header[3];
>      int lpcm_align;

is the lpcm stuff mpeg-PS specific? if not then it does belong in the PES stuff


[...]
>  
>      id = stream->id;
> -
>  #if 0
>      printf("packet ID=%2x PTS=%0.3f\n",
>             id, pts / 90000.0);

cosmetic


[...]
>  
> -        nb_frames= get_nb_frames(ctx, stream, payload_size - stuffing_size);
[...]
> +                nb_frames= get_nb_frames(ctx, stream, payload_size - stuffing_size);

cosmetic


> +                bytestream_put_byte(&p, nb_frames);
> +                bytestream_put_be16(&p, trailer_size+1);
> +            }
> +        }
> +        if(s->is_mpeg2) {
> +            pes_context->packet_number = s->packet_number;
> +            pes_context->muxer_type = PESMUXER_PS;
> +            if(ff_pes_muxer_write(ctx, stream_index, pts, dts, id, startcode, pes_content, p - pes_content,
> +                 header_len, packet_size, payload_size, stuffing_size) < 0)
> +                return -1;
> +        } else {
>  
>          put_be32(&ctx->pb, startcode);
>  
>          put_be16(&ctx->pb, packet_size);

please split the whole PES muxer code out, not just the mpeg2 specific
part


[...]
> +/**
> + * muxer type for PES
> + */
> +typedef enum {
> +    PESMUXER_PS,
> +    PESMUXER_TS,
> +    PESMUXER_PES
> +} PESMuxerType;
> +

the PES muxer should not need to know in what container the PES stream
will be embeded


[...]
> +/**
> + * Insert a timestamp into the ByteIOContext.
> + * @param[in] pb        the ByteIOContext to be written to
> + * @param[in] id        stream ID
> + * @param[in] timestamp the timestamp
> + * @return  NULL
> + */
> +inline void put_timestamp(ByteIOContext *pb, int id, int64_t timestamp);

this wont compile on a non gnu c compiler


also i think it would be cleaner to split the PES muxer into a proper
AVOutputFormat

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

There will always be a question for which you do not know the correct awnser.
-------------- 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/20070831/b800c18b/attachment.pgp>