`-cache` causes playbacks fail to start

Fedyon commented 4 months ago

Sometimes this happens:

MPlayer sherpya-r38203+gb51b9bbd42-10-win32 (C) 2000-2020 MPlayer Team

Playing ***.mkv.
CACHE_PRE_INIT: 0 [0] 0  pre:251658  eof:0  

Cache fill:  0.00% (0 bytes)   

libavformat version 58.64.100 (internal)
libavformat file format detected.
CACHE2_SEEK: 0x0 <= 0x6AF010DF (0x8000) <= 0x120000  
Out of boundaries... seeking to 0x6AF01000  
Seek done. new pos: 0x6AF01000  
CACHE2_SEEK: 0x6AF01000 <= 0x3605 (0x6AF0137E) <= 0x6AF0137E  
Out of boundaries... seeking to 0x3000  
Seek done. new pos: 0x3000  
cache_stream_seek: WARNING! Can't seek to 0x3605 !
CACHE2_SEEK: 0x3000 <= 0x6AF0137E (0x3000) <= 0x3000  
Out of boundaries... seeking to 0x6AF01000  
Seek done. new pos: 0x6AF01000  
[matroska,webm @ 01b0c540]File ended prematurely at pos. 1794118526 (0x6af0137e)
[matroska,webm @ 01b0c540]Could not find codec parameters for stream 0 (Video: h264, none, 1920x1080): unspecified pixel format

After that, mplayer ends gracefully, without playing anything.

(the used build is somewhat old, due to every newer builds having each own set of bugs hitting my usage, e.g. non-working-at-all or broken MPEG2 or broken Vorbis or something. I can't have it updated still.)

I'm suspecting a race in stream/cache2.c between

cache_stream_seek_long() resets [cache_data]->eof=0; // !!!!!!!
cache_fill(), running at eof, overwrites [cache_data]->eof every now and then which would let cache_read() abort the loop immediately and fail hard.

Due to the nature of the problem, it hardly reproduces on reasonably fast filesystems. I usually get hit by this problem when playing a file on network drives over mildly-choked WiFi.

@rdoeffinger any ideas?

Fedyon commented 3 months ago

Maybe I could have made a fix, sooner than I firstly imagined...

The patch is roughly in two parts:

chunk (4): in cache_read(), act on s->eof only when the cache is valid and empty forwards.
in cache_fill(), clear s->eof before updating s->max_filepos. -- chunks (1)(2): add s->eof=0; before cache_flush(s). reorder properly. -- chunk (3): near the end, fix s->eof= !len; which is reordered into the middle of s->max_filepos+=len;.

The idea is to make ->eof signaling strictly one-way: write in filler, read in reader. Halfway mixture is not going to work. Note that in cache_stream_seek_long(), now s->eof=0; // !!!!!!! should be redundant, but is left as-is.

Patch for mplayer.exe r38203-i7:

(1)
+  56f422:  66 0f d6 7d 48          movq   QWORD PTR [ebp+0x48],xmm7
+  56f427:  66 0f d6 3e             movq   QWORD PTR [esi],xmm7
+  56f42b:  66 0f d6 3f             movq   QWORD PTR [edi],xmm7
+  56f42f:  c3                      ret    

(2)
   56f4dc:  39 c6                   cmp    esi,eax
   56f4de:  19 d7                   sbb    edi,edx
   56f4e0:  0f 8c 8d 00 00 00       jl     0x56f573
-  56f4e6:  8b 45 50                mov    eax,DWORD PTR [ebp+0x50]
-  56f4e9:  8b 55 54                mov    edx,DWORD PTR [ebp+0x54]
-  56f4ec:  89 45 40                mov    DWORD PTR [ebp+0x40],eax
-  56f4ef:  89 45 38                mov    DWORD PTR [ebp+0x38],eax
-  56f4f2:  89 45 48                mov    DWORD PTR [ebp+0x48],eax
+  56f4e6:  f3 0f 7e 7d 50          movq   xmm7,QWORD PTR [ebp+0x50]
+  56f4eb:  8d 75 40                lea    esi,[ebp+0x40]
+  56f4ee:  8d 7d 38                lea    edi,[ebp+0x38]
+  56f4f1:  7c 02                   jl     0x56f4f5
+  56f4f3:  87 fe                   xchg   esi,edi
   56f4f5:  8b 45 58                mov    eax,DWORD PTR [ebp+0x58]
-  56f4f8:  89 55 44                mov    DWORD PTR [ebp+0x44],edx
-  56f4fb:  89 55 3c                mov    DWORD PTR [ebp+0x3c],edx
-  56f4fe:  89 55 4c                mov    DWORD PTR [ebp+0x4c],edx
-  56f501:  8b 70 48                mov    esi,DWORD PTR [eax+0x48]
-  56f504:  85 f6                   test   esi,esi
+  56f4f8:  31 d2                   xor    edx,edx
+  56f4fa:  89 55 30                mov    DWORD PTR [ebp+0x30],edx
+  56f4fd:  e8 20 ff ff ff          call   0x56f422
+  56f502:  39 54 10 48             cmp    DWORD PTR [eax+edx*1+0x48],edx
   56f506:  0f 85 c4 05 00 00       jne    0x56fad0

(3)
   56f7c5:  09 d8                   or     eax,ebx
   56f7c7:  0f 94 c0                sete   al
-  56f7ca:  01 4d 40                add    DWORD PTR [ebp+0x40],ecx
-  56f7cd:  0f b6 c0                movzx  eax,al
-  56f7d0:  11 5d 44                adc    DWORD PTR [ebp+0x44],ebx
-  56f7d3:  89 45 30                mov    DWORD PTR [ebp+0x30],eax
+  56f7ca:  0f b6 c0                movzx  eax,al
+  56f7cd:  89 45 30                mov    DWORD PTR [ebp+0x30],eax
+  56f7d0:  01 4d 40                add    DWORD PTR [ebp+0x40],ecx
+  56f7d3:  11 5d 44                adc    DWORD PTR [ebp+0x44],ebx

(4)
   570136:  8b 45 50                mov    eax,DWORD PTR [ebp+0x50]
   570139:  8b 55 54                mov    edx,DWORD PTR [ebp+0x54]
-  57013c:  39 f0                   cmp    eax,esi
-  57013e:  89 d3                   mov    ebx,edx
-  570140:  19 fb                   sbb    ebx,edi
-  570142:  7d 0e                   jge    0x570152
-  570144:  3b 45 38                cmp    eax,DWORD PTR [ebp+0x38]
-  570147:  89 d3                   mov    ebx,edx
-  570149:  1b 5d 3c                sbb    ebx,DWORD PTR [ebp+0x3c]
-  57014c:  0f 8d ae 00 00 00       jge    0x570200
-  570152:  8b 5d 30                mov    ebx,DWORD PTR [ebp+0x30]
-  570155:  85 db                   test   ebx,ebx
+  57013c:  3b 45 38                cmp    eax,DWORD PTR [ebp+0x38]
+  57013f:  8b da                   mov    ebx,edx
+  570141:  1b 5d 3c                sbb    ebx,DWORD PTR [ebp+0x3c]
+  570144:  7c 17                   jl     0x57015d
+  570146:  3b c6                   cmp    eax,esi
+  570148:  8b da                   mov    ebx,edx
+  57014a:  1b df                   sbb    ebx,edi
+  57014c:  0f 8c ae 00 00 00       jl     0x570200
+  570152:  7f 09                   jg     0x57015d
+  570154:  3b 5d 30                cmp    ebx,DWORD PTR [ebp+0x30]
   570157:  0f 85 93 02 00 00       jne    0x5703f0

Patch for mplayer.exe r38203-k8:

(1)
+  580492:  66 0f d6 7d 48          movq   QWORD PTR [ebp+0x48],xmm7
+  580497:  66 0f d6 3e             movq   QWORD PTR [esi],xmm7
+  58049b:  66 0f d6 3f             movq   QWORD PTR [edi],xmm7
+  58049f:  c3                      ret    

(2)
   580558:  39 c6                   cmp    esi,eax
   58055a:  19 d7                   sbb    edi,edx
   58055c:  0f 8c 8d 00 00 00       jl     0x5805ef
-  580562:  8b 45 50                mov    eax,DWORD PTR [ebp+0x50]
-  580565:  8b 55 54                mov    edx,DWORD PTR [ebp+0x54]
-  580568:  89 45 40                mov    DWORD PTR [ebp+0x40],eax
-  58056b:  89 45 38                mov    DWORD PTR [ebp+0x38],eax
-  58056e:  89 45 48                mov    DWORD PTR [ebp+0x48],eax
+  580562:  f3 0f 7e 7d 50          movq   xmm7,QWORD PTR [ebp+0x50]
+  580567:  8d 75 40                lea    esi,[ebp+0x40]
+  58056a:  8d 7d 38                lea    edi,[ebp+0x38]
+  58056d:  7c 02                   jl     0x580571
+  58056f:  87 fe                   xchg   esi,edi
   580571:  8b 45 58                mov    eax,DWORD PTR [ebp+0x58]
-  580574:  89 55 44                mov    DWORD PTR [ebp+0x44],edx
-  580577:  89 55 3c                mov    DWORD PTR [ebp+0x3c],edx
-  58057a:  89 55 4c                mov    DWORD PTR [ebp+0x4c],edx
-  58057d:  8b 48 48                mov    ecx,DWORD PTR [eax+0x48]
-  580580:  85 c9                   test   ecx,ecx
+  580574:  31 d2                   xor    edx,edx
+  580576:  89 55 30                mov    DWORD PTR [ebp+0x30],edx
+  580579:  e8 14 ff ff ff          call   0x580492
+  58057e:  39 54 10 48             cmp    DWORD PTR [eax+edx*1+0x48],edx
   580582:  0f 85 f0 05 00 00       jne    0x580b78

(3)
   580843:  89 f0                   mov    eax,esi
   580845:  09 f8                   or     eax,edi
   580847:  0f 94 c0                sete   al
-  58084a:  01 75 40                add    DWORD PTR [ebp+0x40],esi
-  58084d:  0f b6 c0                movzx  eax,al
-  580850:  11 7d 44                adc    DWORD PTR [ebp+0x44],edi
-  580853:  01 f1                   add    ecx,esi
-  580855:  89 45 30                mov    DWORD PTR [ebp+0x30],eax
+  58084a:  0f b6 c0                movzx  eax,al
+  58084d:  89 45 30                mov    DWORD PTR [ebp+0x30],eax
+  580850:  01 75 40                add    DWORD PTR [ebp+0x40],esi
+  580853:  11 7d 44                adc    DWORD PTR [ebp+0x44],edi
+  580856:  01 f1                   add    ecx,esi

(4)
   5811bc:  8b 43 50                mov    eax,DWORD PTR [ebx+0x50]
   5811bf:  8b 53 54                mov    edx,DWORD PTR [ebx+0x54]
-  5811c2:  39 f0                   cmp    eax,esi
-  5811c4:  89 d1                   mov    ecx,edx
-  5811c6:  19 f9                   sbb    ecx,edi
-  5811c8:  7d 0e                   jge    0x5811d8
-  5811ca:  3b 43 38                cmp    eax,DWORD PTR [ebx+0x38]
-  5811cd:  89 d1                   mov    ecx,edx
-  5811cf:  1b 4b 3c                sbb    ecx,DWORD PTR [ebx+0x3c]
-  5811d2:  0f 8d b0 00 00 00       jge    0x581288
-  5811d8:  8b 6b 30                mov    ebp,DWORD PTR [ebx+0x30]
-  5811db:  85 ed                   test   ebp,ebp
+  5811c2:  3b 43 38                cmp    eax,DWORD PTR [ebx+0x38]
+  5811c5:  8b ca                   mov    ecx,edx
+  5811c7:  1b 4b 3c                sbb    ecx,DWORD PTR [ebx+0x3c]
+  5811ca:  7c 17                   jl     0x5811e3
+  5811cc:  3b c6                   cmp    eax,esi
+  5811ce:  8b ca                   mov    ecx,edx
+  5811d0:  1b cf                   sbb    ecx,edi
+  5811d2:  0f 8c b0 00 00 00       jl     0x581288
+  5811d8:  7f 09                   jg     0x5811e3
+  5811da:  3b 4b 30                cmp    ecx,DWORD PTR [ebx+0x30]
   5811dd:  0f 85 dd 02 00 00       jne    0x5814c0

Now the problem lies: I'm not sure how to describe the fix into the C source! The code proudly has no locks, no barriers, no atomics, nothing whatever. Where to begin? In modern sense, it smells wrong by definition; and in reality, it is not correct enough.

Fedyon commented 3 months ago

Correction: chunk (2)'s last 85 54 10 48 test ... should be 39 54 10 48 cmp ... (edit: changes applied)

Note that console outputs are way too slow and tend to hide the issue, especially with increased -msglevel cache. Redirect stdout to a file, or run as -slave under SMplayer.

sherpya / MPlayer

`-cache` causes playbacks fail to start #24