[Ffmpeg-devel] [PATCH] MP3 decoding from RTP stream. Getting "invalid new backstep"

[Martin Thielen]

Hi

Michael Niedermayer wrote:
> Hi
> 
> On Fri, Nov 03, 2006 at 03:20:04PM +0100, Martin wrote:
> 
>>Michael Niedermayer wrote:
>>
>>>Hi
>>>
>>>On Fri, Nov 03, 2006 at 12:25:30PM +0100, Martin wrote:
>>>
>>>
>>>>Hi,
>>>>
>>>>
>>>>
>>>>>>right before returning:
>>>>>>
>>>>>>if (discard_frame) {
>>>>>>  //dicard frame is set if an backstep or overread occurs.
>>>>>>  for(i=0;i<nb_frames;i++) {
>>>>>>    for(ch=0;ch<s->nb_channels;ch++) {
>>>>>>      int j;
>>>>>>      for(j=0;j<SBLIMIT;j++) {
>>>>>>        s->sb_samples[ch][i][j] = 0;
>>>>>>      }
>>>>>>    }
>>>>>>  }
>>>>>>}
>>>>>>
>>>>>>
>>>>>>
>>>>>>This didn't worked as expected. Also I found another problem caused by
>>>>>>this point in the huffman_decode() function:
>>>>>>
>>>>>>if (pos > end_pos2 && last_pos){
>>>>>>              /* some encoders generate an incorrect size for this
>>>>>>                 part. We must go back into the data */
>>>>>>              s_index -= 4;
>>>>>>              skip_bits_long(&s->gb, last_pos - pos);
>>>>>>              av_log(NULL, AV_LOG_INFO, "overread, skip %d enddists:
>>>>>>%d %d\n", last_pos - pos, end_pos-pos, end_pos2-pos);
>>>>>>              s->frame_distorted = 1;
>>>>>>
>>>>>>              break;
>>>>>>          }
>>>>>>
>>>>>>
>>>>>>My idea was now to introduce a new flag into the MPADecodeContext:
>>>>>>int frame_distorted;
>>>>>>
>>>>>>Now I set this flag to 0 at the start of decode_frame()
>>>>>>and set it to 1 when backstep or overread occurs. At the end of
>>>>>>decode_frame() I check the flag and return -1 if it is set. This is
>>>>>>working for me. If I send a patch with the new flag, would you include it?
>>>>>
>>>>>
>>>>>not without understanding why setting sb_samples to 0 doesnt work
>>>>>
>>>>>[...]
>>>>
>>>>It seems to work now, with setting sb_frames to 0. I did it after the
>>>>synthesis filter which didn't work. How should I transmit the fact that
>>>>a distortion occured in huffmann_decode() to the function
>>>>mp_decode_frame()? Can I use the flag for this purpose?
>>>>I'm not sure whether I break something if I set sb_samples to 0 directly
>>>>at the point where the distortion occurs in huffmann_decode().
>>>
>>>
>>>huffmann_decode() should return -1 if theres an error
>>>
>>>[...]
>>
>>Thanks for the help!
>>So here is a patch which sets buffers to 0 if 'overread' or 'backstep'
>>occurs which prior resulted in distorted sound.
>>
>>Martin
> 
> 
>>--- mpegaudiodec.c_bak	2006-11-03 14:38:48.263103729 +0100
>>+++ mpegaudiodec.c	2006-11-03 14:43:02.153241601 +0100
>>@@ -1705,7 +1705,8 @@ static int huffman_decode(MPADecodeConte
>>                 s_index -= 4;
>>                 skip_bits_long(&s->gb, last_pos - pos);
>>                 av_log(NULL, AV_LOG_INFO, "overread, skip %d enddists: %d %d\n", last_pos - pos, end_pos-pos, end_pos2-pos);
>>-                break;
>>+                //break;
>>+                return -1;
>>             }
> 
> 
> this one will break decoding of some mp3 files (from broken encoders ...)

So ahould I set the samples to 0 here as well instead of return -1?

> 
> 
> [...]
> 
>>         assert(i <= buf_size - HEADER_SIZE && i>= 0);
>>         memcpy(s->last_buf + s->last_buf_size, s->gb.buffer + buf_size - HEADER_SIZE - i, i);
>>@@ -2503,6 +2508,19 @@ static int mp_decode_frame(MPADecodeCont
>>         }
>>     }
>> #endif
>>+
>>+    // zero samples if frame is distorted by overread or backstep
>>+    if (discard_frame) {
>>+        for(i=0;i<nb_frames;i++) {
>>+            for(ch=0;ch<s->nb_channels;ch++) {
>>+                int j;
>>+                for(j=0;j<SBLIMIT;j++) {
>>+                    s->sb_samples[ch][i][j] = 0;
>>+                }
>>+            }
>>+        }
>>+    }
> 
> 
> memset(0);
> 
> [...]

Is the rest of the patch ok?

Martin