[FFmpeg-devel] [PATCH]pes packetizer

[Xiaohui Sun]

Michael Niedermayer wrote:
> Hi
>
> On Mon, Aug 27, 2007 at 10:14:59PM +0800, realsun wrote:
> [...]
>   
>> Index: mpeg_pes_enc.c
>> ===================================================================
>> --- mpeg_pes_enc.c	(revision 10145)
>> +++ mpeg_pes_enc.c	(working copy)
>> @@ -1,6 +1,7 @@
>>  /*
>> - * MPEG1/2 muxer
>> - * Copyright (c) 2000, 2001, 2002 Fabrice Bellard.
>> + * MPEG PES muxer
>> + * Copyright (c) 2000-2002 Fabrice Bellard
>> + * Copyright (c) 2007 Xiaohui Sun <sunxiaohui at dsp.ac.cn>
>>   *
>>   * This file is part of FFmpeg.
>>   *
>>     
>
> iam not sure if spliting a file makes you legally a copyright holder of the
> result
>
>   

  i don't intend to exaggerate my work, removed

> diff -uw ~/ffmpeg-svn/trunk/libavformat/mpegenc.c mpeg_pes_enc.c|diffstat
>  mpeg_pes_enc.c | 1095 ++-------------------------------------------------------
>  1 file changed, 52 insertions(+), 1043 deletions(-)
> wc mpeg_pes_enc.c
>  299  953 9773 mpeg_pes_enc.c
>
> the 52 lines are largely function renamings and cosmetic changes which to a
> large extend do not belong in this patch and which i have complained about in
> past reviews
>   

sorry for my careless, haste makes waste ;-)

> [...]
>   
>>              break;
>>          case CODEC_TYPE_VIDEO:
>> -            stream->id = mpv_id++;
>> -            if (st->codec->rc_buffer_size)
>> -                stream->max_buffer_size = 6*1024 + st->codec->rc_buffer_size/8;
>> -            else
>> -                stream->max_buffer_size = 230*1024; //FIXME this is probably too small as default
>>  #if 0
>>                  /* see VCD standard, p. IV-7*/
>>                  stream->max_buffer_size = 46 * 1024;
>> @@ -368,10 +48,12 @@
>>                     Right now it is also used for everything else.*/
>>                  stream->max_buffer_size = 230 * 1024;
>>  #endif
>> -            s->video_bound++;
>> +            if (st->codec->rc_buffer_size)
>> +                stream->max_buffer_size = 6*1024 + st->codec->rc_buffer_size/8;
>> +            else
>> +                stream->max_buffer_size = 230*1024; //FIXME this is probably too small as default
>>              break;
>>     
>
> ive already in a past review said that this cosmetic change is not ok
> you should not move the code over the #if 0 #endif
>   
fixed

>
> [...]
>   
>>  }
>>  
>> -static inline void put_timestamp(ByteIOContext *pb, int id, int64_t timestamp)
>> +void ff_insert_timestamp(ByteIOContext *pb, int id, int64_t timestamp)
>>  {
>>      put_byte(pb,
>>               (id << 4) |
>>     
>
> as said in past reviews the function renaming should be a seperate patch
>   
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 */
>>     
>
> as already said in past reviews, this is a cosmetic change and does not
> belong in here, please do not reindent the code
>   
fixed

>
> [...]
>   
>> @@ -868,128 +134,30 @@
>>                      put_be16(&ctx->pb, 0x4000 | stream->max_buffer_size/128);
>>                  else
>>                      put_be16(&ctx->pb, 0x6000 | stream->max_buffer_size/1024);
>> -            }
>> +    }
>>  
>>     
>
> ive also complained about this reindention in past reviews already
>
>   
fixed

> [...]
>   
>> -        if (s->is_mpeg2) {
>>              /* special stuffing byte that is always written
>> -               to prevent accidental generation of start codes. */
>> +               to prevent accidental generation of startcodes. */
>>              put_byte(&ctx->pb, 0xff);
>>  
>>     
>
> this too ive complained about already, why do you ignore the comments
> people post about your patches?
>   
some people complained that I should use startcode instead of start 
codes, I forgot to move this change out of this patch

>
> [...]
>   
>>  }
>>  
>> -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...

> [...]
>
> you have ignored every single comment from the review from (07.29  2:12)
> and have only taken care of some comments but not all from older reviews
> though ive not exhaustively checked the older reviews
>
>   
> ------------------------------------------------------------------------
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at mplayerhq.hu
> http://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel

-------------- next part --------------
A non-text attachment was scrubbed...
Name: pes.diff
Type: text/x-patch
Size: 67603 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070831/294e5d8b/attachment.bin>