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

[Nico Sabbi]

Christian Aistleitner wrote:
> Hello,
> 
>>> [ 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


              }
-               return DEMUXER_CTRL_DONTKNOW;
+           return DEMUXER_CTRL_DONTKNOW;


*cosmetics

-  mpg_demuxer_t* mpg_d;
+  mpg_demuxer_t* mpg_d=demuxer->priv;

*not strictly cosmetics, but you can avoid it because it's confusing


> 
> I could not find a document describing MPlayer's coding conventions.
> Can you please point me towards what you mean by "cosmetics overflow"

coding conventions are the usual 2:
-code that doesn't change semantics qualifies as cosmetics
-cosmetics must be split from semantical changes

> 
> I added two comments to existing members of the mpg_demuxer.
> One for last_pts, as without the comment it is not obvious whether or 
> not  it refers to the most recently found one or the last one in the 
> stream.
> Another comment has been applied to has_valid_timestamps as its name 
> daes  not describe what it means and additionally my patch gives it a 
> breader  more well defined sense.
> 
> Are you refering to that?

also

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

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

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

-- 
"Without a frontend, mplayer is useless" - someone in mplayer-users