[MPlayer-dev-eng] [PATCH] audio file truncation

[Uoti Urpala]

On Tue, 2006-06-06 at 13:56 -0400, Alan Curry wrote:
> >> +    if(sh_audio->a_in_buffer_len==0 && sh_audio->a_buffer_len==0)
> >> +      playflags |= AOPLAY_FINAL_CHUNK;
> >
> >This is wrong. You can't detect the final chunk this way; it's normal
> >for those buffers to be empty in the middle of playback.
> 
> Well, I'm not surprised that my solution wasn't the best possible. It did
> help in my test cases though.

It's clearly wrong, not just "not the best possible". Writing the test
like that will often set AOPLAY_FINAL_CHUNK unnecessarily, in many cases
for every play() call. If you think the ao drivers should always behave
as if the flag was set then adding the flag is pointless.

> >-  if(d_audio->eof && sh_audio->a_in_buffer_len <= 0 && sh_audio->a_buffer_len <= 0) eof = PT_NEXT_ENTRY;
> >+  if(d_audio->eof && sh_audio->a_in_buffer_len <= 0 && sh_audio->a_buffer_len <= 0 && sh_audio->a_out_buffer_len <= 0) eof = PT_NEXT_ENTRY;
> >
> >While the change here isn't for the worse, the original and resulting
> >line are questionable. It's not impossible for the decoder to have some
> >internal buffering which cannot be seen from public buffer sizes. In my
> >opinion this code should not attempt to detect EOF before decode_audio()
> >returns <= 0.
> 
> So this is another case of

Cut sentence? Anyway here's a way to implement this test and the
AOPLAY_FINAL_CHUNK test based on return value of decode_audio(). Not a
complete patch and untested but hopefully it shows the method.

@@ -3619,6 +3619,8 @@
   unsigned int t;
   double tt;
   int playsize;
+  int playflags=0;
+  int audio_eof=0;

   current_module="play_audio";

@@ -3637,24 +3639,31 @@
   // Fill buffer if needed:
   current_module="decode_audio";   // Enter AUDIO decoder module
   t=GetTimer();
-  while(sh_audio->a_out_buffer_len<playsize &&
-        (!d_audio->eof || sh_audio->a_in_buffer_len > 0 || sh_audio->a_buffer_len > 0)){
+  while (sh_audio->a_out_buffer_len < playsize) {
     int ret=decode_audio(sh_audio,&sh_audio->a_out_buffer[sh_audio->a_out_buffer_len],
         playsize-sh_audio->a_out_buffer_len,sh_audio->a_out_buffer_size-sh_audio->a_out_buffer_len);
     if(ret<=0) { // EOF?
-      if (d_audio->eof)
+      if (d_audio->eof) {
+        audio_eof = 1;
+        if (!sh_video && sh_audio->a_out_buffer_len == 0)
+           eof = PT_NEXT_ENTRY;
         sh_audio->a_in_buffer_len = 0; // make sure we don't hang if something's broken
+      }
       break;
     }
     sh_audio->a_out_buffer_len+=ret;
   }
   t=GetTimer()-t;
   tt = t*0.000001f; audio_time_usage+=tt;
-  if(playsize>sh_audio->a_out_buffer_len) playsize=sh_audio->a_out_buffer_len;
+  if(playsize > sh_audio->a_out_buffer_len) {
+      playsize=sh_audio->a_out_buffer_len;
+      if (audio_eof)
+         playflags |= AOPLAY_FINAL_CHUNK;
+  }

   // play audio:
   current_module="play_audio";
-  playsize=audio_out->play(sh_audio->a_out_buffer,playsize,0);
+  playsize=audio_out->play(sh_audio->a_out_buffer, playsize, playflags);

   if(playsize>0){
       sh_audio->a_out_buffer_len-=playsize;
@@ -3671,8 +3680,6 @@
     float a_pos = sh_audio->delay - audio_out->get_delay() * playback_speed;
     print_status(a_pos, 0, 0);
   }
-  if(d_audio->eof && sh_audio->a_in_buffer_len <= 0 && sh_audio->a_buffer_len <= 0) eof = PT_NEXT_ENTRY;
-
 } else {

 /*========================== PLAY VIDEO ============================*/



> >AOPLAY_FINAL_CHUNK does not mean there's any more need to play the full
> >"len" amount than without the flag. What it does mean is that the ao
> >should not permanently reject the input because of its size; it's still
> >ok to buffer it only partially on this call.
> 
> That's a better way of describing it. Or simply "try not to return 0". And I
> think it's equivalent in the case of my implementation of the flag in ao_oss.

Forbidding return of 0 would be a drawback (there might be lots of
buffered audio already and waiting it to clear first could be sensible
otherwise, so get_space() would have to be careful not to return > 0 in
that situation). It would probably be the easiest way to detect existing
problematic aos (without having to fix them) though.