[MPlayer-dev-eng] [PATCH] Improving handling of PES files

[Nico Sabbi]

Christian Aistleitner wrote:

>>
>> -  mpg_demuxer_t* mpg_d;
>> +  mpg_demuxer_t* mpg_d=demuxer->priv;
>>
>> *not strictly cosmetics, but you can avoid it because it's confusing
> 
> 
> Actually, the problem is the generated diff here, not the code :)
> The subtracted part
> 
>> -  mpg_demuxer_t* mpg_d;
> 
> belongs to the function demux_mpg_open whereas the added part
> 
>> +  mpg_demuxer_t* mpg_d=demuxer->priv;
> 
> belongs to the function read_first_mpeg_pts_at_position. So the 
> generated  diff is misleading here. However, I do not want to hand tune 
> the output of  diff to make it more readable ;)
> 
> So again no cosmetics. And not avoidable.

uhm, the art of patching permits to workaround  diff's stupidity.
See how my attached patch is much more readable than yours, because
it isolates without interleaving removals and additions ;)
It's just to show that patches can be made more readable and much
more easily reviewable, not only in -dev-eng but in svn log, too.
I don't have anything against you personally

> 
> 
>> many bad things can happen at open(); there where recent reports of  
>> hangs during open() because of seek() over http
> 
> 
> Ok. But thereby, you do not criticise my patch, but the original code 
> in  mplayer. I've not written the MPlayer's current code :)
> 

if it's of any consolation demux_mpg.c is not my craft, either

>>>  I'd rather go for three initial seeks which give me better seeking   
>>> accuracy and runtime information instead of two wasted seeks as for   
>>> MPlayer without my patch.
>>
>>
>> I'll apply this patch only limited to STREAMTYPE_FILE (!= pipe)
> 
> 
> That's a good idea. Yes, I included that in the attached patch. 
> However,  note that you are not nagging around on my patch but the 
> MPlayer's code  some else wrote ;)
> 

indeed it's a horrible mess that I can't really stand.

Attached patch contains a couple of nits and one fix:
- after calloc() there's no need to set struct members to 0;
- in the whole file the style
if ( x )
is never used, so I removed it from your patch, too
- in read_first_mpeg_pts_at_position() something incorrect may lead
to an wrong failure:
a) timestamps can go backwards without necessarily having reset
(e.g. try to run mencoder with -of mpeg -mpegopts format=dvd:tsaf
and include in the video stream 1+ b-frames);
without using fabsf() your previous function would fail; with fabsf()
it works correctly
b) the 3 timestamps must be inited to mpg_d->last_pts, not -1,
and after the first valid timestamp found they have to be inited again 
   to
the same value, or -1 - valid_timestamp may be > 
MAX_PTS_DIFF_FOR_CONSECUTIVE


If you find something you don't like let me know soon;
I plan to commit this new patch tomorrow

-- 
"Without a frontend, mplayer is useless" - someone in mplayer-users
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pts.diff
Type: text/x-patch
Size: 8338 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20070127/82ae3d49/attachment.bin>