[MPlayer-dev-eng] MPlayer MPEG4 Seeking Bug (and patch)

[Andyz Smith]

Ubuntu 10.04
Mplayer 1.0rc4
Encore TV Card

I use mencoder to capture video from the TV card and encode it to MPEG4.

I open the resulting file with GMplayer, while it's still growing to achieve a DVR like effect.

I noticed that the seek behaviour with MPEG4 was quite poor.  Major distortion of the image was present for several seconds after seeking forward 10 seconds or selecting a random position with the slider.

Encode to MPEG2 worked much better.  
MPEG4 with a keyint of 10 also made large improvements. 

Neither of those is really a solution.  

Upon review of the mplayer source code I believe the following is a rather large bug in the MPlayer MPEG4 decoder/demuxer seeking.  My suggested patch and comments (AJS) follow.

demux_mpg.c
ln 980

if(sh_video->format == 0x10000004) {    //mpeg4
            if(i==0x1B6) {            //vop (frame) startcode
          if ( (peek_nal_descriptor & 0xc0)  == 0) // AJS  0x1b6 and then 00xxxxxx binary is an IDR (keyframe)
        break;
        //int pos = videobuf_len;
          //if(!read_video_packet(d_video)) break; // EOF AJS this might lookahed, but I think it reads too far
          //if((videobuffer[pos+4] & 0x3F) == 0) break;    //I-frame AJS this bitwise AND is backwards I think
        }

parse_es.c
ln 38

int videobuf_code_len=0;
int peek_nal_descriptor = -1; //ajs LookAhead for MPEG4 P,B,IDR frame detection

#define MAX_SYNCLEN (10 * 1024 * 1024)

ln 51

    next_nal = demux_getc(ds);
    peek_nal_descriptor = demux_peekc(ds); //ajs LookAhead for MPEG4 P,B,IDR frame detection
    if (next_nal < 0)

ln 96

  next_nal = demux_getc(ds);
  peek_nal_descriptor = demux_peekc(ds); //ajs LookAhead for MPEG4 P,B,IDR frame detection
  if (next_nal < 0)

parse_es.h

ln 30

extern int videobuf_code_len;
extern int peek_nal_descriptor; //ajs LookAhead for MPEG4 P,B,IDR frame detection

// sync video stream, and returns next packet code

Ok, So

The MPEG4 Part 2 spec defines frame type using AN EXTRA BYTE.  It seems that neither MPEG2 nor H264 ( MPEG Part 10? ) do this.

The first part 0x1B6 specifies only that the frame is a VOP (Video Object Plane). It could be differential compression frame (B) or predicted frame (P) or a actual full picture (IDR, Immediate Decoder Refresh, keyframe).

The next byte specifies which kind of frame. During seeking we only want IDR(keyframes), otherwise distortion will be large until the next keyframe.

Ok... The code to check for this is in there, but it doesn't seem to work.

Specifically, this appears quite wrong and broken.

if((videobuffer[pos+4] & 0x3F) == 0)

The spec says that the higher 2 bits from this byte determine the frame type.

VOP_CODING_TYPE (binary)  Coding method
                      00  intra-coded (I)
                      01  predictive-coded (P)
                      10  bidirectionally-predictive-coded (B)
                      11  sprite (S)

So to find out if this definitely is an IDR(keyframe) we must:

videobuffer[pos+4] & 0xC0

00xxxxxx & 00111111 (x3F) Doesnt tell us anything

00xxxxxx & 11000000 (xC0) Will be 00000000 (==0) if and only if it's an IDR(keyframe). 

On to the next.

Since the other encoding methods (MPEG2 and H264) don't use this extra byte we have to go and get it special.

Specifically, this seems designed to do this.   An extra read inside of the main read loop.

//if(!read_video_packet(d_video)) break; // EOF

But if you look at the code for read_video_packet, it seems to only 'copy the startcode' for the first bytes.

  // COPY STARTCODE:
  packet_start=videobuf_len;
  videobuffer[videobuf_len+0]=0;
  videobuffer[videobuf_len+1]=0;
  videobuffer[videobuf_len+2]=1;
  videobuffer[videobuf_len+3]=next_nal;
videobuf_len+=4;

SOOOO, not only is x3F the wrong bit-mask, the data won't be there anyway. And, to compound the problem, this extra read_video_packet PROBABLY does something funky to the state of the stream.  Not sure. Anyway.

My solution/hack.

Super Global variable "peek_nal_descriptor"
Set this whenever getting data, but don't mess up the state of the stream..use "demux_peekc".

next_nal = demux_getc(ds);
    peek_nal_descriptor = demux_peekc(ds);

Use super global variable in seek code.

    if(i==0x1B6) {
          if ( (peek_nal_descriptor & 0xc0)  == 0)
        break;

OK, good.

Works.

Furthermore...

AFAIK, this code is only for skipping packets until an I-frame.  It seems no provision is made for re-generating a picture by skipping backward to the previous I-frame and applying the B and P differential frames.  Code for such a feature would certainly break this whole deal.

I don't like the global variable any more than anyone else, but this is C, kind of old crusty C....anyway

It would be better if this were more integrated in the flow of parse_es.c.   Like if read_video_packet set the rest of the structure.  Or if sync_video_packet did something to set up the structure.
This would probably be part of the problem.  Return value from sync_video_packet.

return 0x100|next_nal;

Only room for one byte.

I think demux_ts.c has the same problem.



Allright, let me know what you think.

Gotta go watch *hicago *ode on my 'DVR'

           
andyzsmith at yahoo.com