[FFmpeg-devel] [PATCH]pes packetizer

[Michael Niedermayer]

Hi

On Sat, Sep 01, 2007 at 05:58:09PM +0800, Xiaohui Sun wrote:
> Michael Niedermayer wrote:
>> 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 */
>>>     
>>
>>
>> [...]
>>
>>   
>>>>>  }
>>>>>  -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)
>>
>>   
>
> yes I used the original function name except adding a ff_ prefix

yes, and the function renaming MUST be in a seperate patch and they
MUST be actually needed


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

no you have not fixed it, i told you many times that function renamings
MUST be in seperate patches
dont pretend that you dont understand me!

also as this is getting nowhere i tried to split the code myself just to
realize that this function is only used in the PES code so there is no
need for it to be non static

this function must stay in a single file and it must stay static
it must not be duplicated
you must split the _WHOLE_ pes code out not just the mpeg2 code (i also have
told you this already)


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

no the line is still changed


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No great genius has ever existed without some touch of madness. -- Aristotle
-------------- 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/20070905/d6eadc1a/attachment.pgp>