[FFmpeg-devel] [PATCH]pes packetizer

[Michael Niedermayer]

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

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


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


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


[...]

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


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


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


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

[...]

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

-- 
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/20070830/5673d004/attachment.pgp>