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

[Michael Niedermayer]

Hi

On Mon, Nov 06, 2006 at 09:19:17AM +0100, Martin Thielen wrote:
> 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?

no this case is not an error


> 
> > 
> > 
> > [...]
> > 
> >>         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?

i think so, but thats just from memory ...

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In the past you could go to a library and read, borrow or copy any book
Today you'd get arrested for mere telling someone where the library is