[MPlayer-cvslog] r37309 - trunk/stream/stream.h

[Ivan Kalvachev]

On 12/3/14, Reimar Döffinger <Reimar.Doeffinger at gmx.de> wrote:
> On Wed, Dec 03, 2014 at 09:24:18AM +0100, Reimar Döffinger wrote:
>> On 02.12.2014, at 22:22, Ivan Kalvachev <ikalvachev at gmail.com> wrote:
>> > On 10/25/14, reimar <subversion at mplayerhq.hu> wrote:
>> >> Author: reimar
>> >> Date: Sat Oct 25 17:27:08 2014
>> >> New Revision: 37309
>> >>
>> >> Log:
>> >> stream: Avoid unnecessary seek when our buffer is empty.
>> >>
>> >> Modified:
>> >>   trunk/stream/stream.h
>> >>
>> >> Modified: trunk/stream/stream.h
>> >> ==============================================================================
>> >> --- trunk/stream/stream.h    Sat Oct 25 15:40:40 2014    (r37308)
>> >> +++ trunk/stream/stream.h    Sat Oct 25 17:27:08 2014    (r37309)
>> >> @@ -324,6 +324,8 @@ static inline int stream_seek(stream_t *
>> >>            "Invalid seek to negative position %"PRIx64"!\n", pos);
>> >>     pos = 0;
>> >>   }
>> >> +  if (s->buf_len == 0 && s->pos == pos)
>> >> +    return 1;
>> >>   if(pos<s->pos){
>> >>     int64_t x=pos-(s->pos-s->buf_len);
>> >>     if(x>=0){
>> >
>> > This breaks sub/subreader.c::guess_cp() code.
>> > The code reads the file once and feeds it to enca, in order to guess
>> > the encoding/code_page. Then it calls:
>> >    stream_reset();
>> >    stream_seek();.
>> >
>> > With the above change, data structures are changed, but actual file
>> > seek is not performed. So when subtitle parsing really starts there is
>> > nothing to parse.
>> >
>> > The interesting part is that stream_reset() have a commented out call
>> > to stream_seek(), in order to ensure actual seek, but I guess it have
>> > been removed as it is unnecessary in the most common case (opening
>> > file).
>
> I think the intention is that stream_reset actually should only
> reset the buffers etc. but not actually seek.
>
>> > Please revert.
>>
>> Reverting it breaks auto-detection with non-seekable inputs, thus is not a
>> real solution either.
>
> Does the below work? It looks to me like it should be correct, and the pos=0
> assignment
> is from a very early revision, so there's no indication it is actually
> required
> for anything from the logs.
>
> --- a/stream/stream.c
> +++ b/stream/stream.c
> @@ -482,7 +482,6 @@ while(stream_fill_buffer(s) > 0 && pos >= 0) {
>
>  void stream_reset(stream_t *s){
>    if(s->eof){
> -    s->pos=0;
>      s->buf_pos=s->buf_len=0;
>      s->eof=0;
>    }

Sorry I missed your patch.
I just tested it and it seems to work.

Feel free to commit it. If I encounter problem I'll tell you right away.