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

[Christian Aistleitner]

Hello,

in the attachment you'll find a revised version of the patch from the mail  
starting this thread.
1 cosmetic problem has been treated.
Some faulty checks to 1.0 have been replaced by checks to -1.0
Computation of has_valid_timestamps (ie 3 seeks) has been limited to files.

>>>> [ short description of patch ]
>>>
>>>
>>> there are two problems with this patch:
>>> 1) cosmetics overflow
>>   I do not really understand that point.
>> I did not tamper with the spacings of existing code.
>> I did not change positions of braces.
>> I did not rename of variables.
>
>   typedef struct mpg_demuxer {
> -  float last_pts;
> -  float final_pts;
> -  int has_valid_timestamps;
> +  float last_pts;               // most recently found pts
> +  float first_pts;              // first pts found in stream
>
> *cosmetics

No cosmetics to me. But I argued that the comment to last_pts is  
necessary. The other comments describe the canged semantics.

>               }
> -               return DEMUXER_CTRL_DONTKNOW;
> +           return DEMUXER_CTRL_DONTKNOW;
>
>
> *cosmetics

Ja. Sorry. I looked through the patch several times. I seem to have  
overlooked this. This is cosmetics. It has been removed in the attached  
patch.

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


So there has been only one cosmetic problem. I removed this issue in the  
attached patch.

>>> 2) too much seeking (unacceptable, think of what happens on non-file   
>>> streams) and too much complication (that is not worth the result, in  
>>> any  case).
>>>
>>> If you want precise seeking you should implement -idx instead
>>   Currently MPlayer performs two seeks (although these are rather   
>> obfuscated). The first seek ggoes to half_pos, the second near the end  
>> of  the stream. For the PES files I record, these two seeks never lead  
>> to  "has_valid_timestamps" being true, although my timestamps are  
>> neither  wrong nor fancy.
>
> 2 seeks are already too many, 3 are worse

But bear with me that my first seeking of pts happens at the beginning of  
the stream, so the first seek is to the start of the stream. The stream is  
typically at this position. So this seek should not do too much harm.  
However, 3 seeks are more than 2. Definitely.

>>  My patch performs three seeks (i.e.: one additional seek). I made  
>> these  seeks more evident, as I am factored the seeking code into the  
>> new  function read_first_mpeg_pts_at_position. However, with these  
>> three seeks  all of my PES files (except for those where the timestamp  
>> overruns) have  "has_valid_timestamps" set to true. And additionally, I  
>> know the runtime  of the file.
>>  These seeks are performed only once when opening the stream.
>
> 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 :)

>>  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 ;)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: improved-pts-validity-checks.diff
Type: application/octet-stream
Size: 8653 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20070125/e6a5ec31/attachment.obj>