View Issue Details

Jump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0000062JVT JM H.264/AVC reference softwaredecoderpublic2007-07-06 04:352011-11-15 21:54
ReporterPeter Farkas Assigned ToKarsten Suehring  
PrioritynormalSeveritymajorReproducibilityalways
Status assignedResolutionopen 
Product VersionJM 12.2 
Target VersionJM 18.2 
Summary0000062: while loops in ldecod.c and image.c do not behave as expected
DescriptionThis may not be a bug strictly speaking, since the decoding seems
to be performed correctly. However, the program does not behave
as one would expect it to behave. I suspect that this is not the
intended behaviour. If it is, then the program should be changed,
since the way it stands now is misleading. Or else, an explanation
should be provided.

Specificly, I am looking at the loops

  while (decode_one_frame(img, input, snr) != EOS)
    ;

in ldecod.c, and

int decode_one_frame(...)
{
  . . . . . . . .
  while ((currSlice->next_header != EOS && currSlice->next_header != SOP))
  {
    . . . . . . .
  }

  exit_picture();

  return (SOP);
}

in image.c.

One would think that each call to decode_one_frame would decode
one frame, untill EOS is reached, and passes through the body inside
the loop in decode_one_frame would decode one frame after which
execution returns to the caller. However, this is not the case.

In reality, decode_one_frame is called only once, and the loop
inside decode_one_frame does the decoding of all the frames.
Indeed, currSlice->next_header never equals SOP.

This can be easily seen with a debugger, or by inserting some
print statements. I modified the loops in the following way:

  int status = 4444;
  while (status != EOS)
  {
    printf("in loop in ldecod.c, counter1 = %d\n", counter1++);
    status = decode_one_frame(img, input, snr);
  }

for the loop in in ldecod.c, and
  
  while ((currSlice->next_header != EOS && currSlice->next_header != SOP))
  {
    printf(
      "beginning of loop in image.c, counter2 = %d "
      "currSlice->next_header = %d\n",
      counter2, currSlice->next_header);
      
      . . . . . . . .
    
    printf(
      "end of loop in image.c, counter2 = %d "
      "currSlice->next_header = %d\n",
      counter2++, currSlice->next_header);
  }

for the loop in in image.c (with the necessary declarations, of
course). The output (to stdout) looks like this:

. . . . . . . .
in loop in ldecod.c, counter1 = 0
beginning of loop in image.c, counter2 = 0 currSlice->next_header = -8888
0000(I) 0 0 28 0.0000 0.0000 0.0000 4:2:0 42
end of loop in image.c, counter2 = 0 currSlice->next_header = -8888
beginning of loop in image.c, counter2 = 1 currSlice->next_header = -8888
0000(P) 2 1 28 0.0000 0.0000 0.0000 4:2:0 14
0000(P) 4 2 28 0.0000 0.0000 0.0000 4:2:0 18
end of loop in image.c, counter2 = 1 currSlice->next_header = -8888
beginning of loop in image.c, counter2 = 2 currSlice->next_header = -8888
0000(P) 6 3 28 0.0000 0.0000 0.0000 4:2:0 12
0000(P) 8 4 28 0.0000 0.0000 0.0000 4:2:0 18
end of loop in image.c, counter2 = 2 currSlice->next_header = -8888
beginning of loop in image.c, counter2 = 3 currSlice->next_header = -8888
0000(P) 10 5 28 0.0000 0.0000 0.0000 4:2:0 12
0000(P) 12 6 28 0.0000 0.0000 0.0000 4:2:0 17
end of loop in image.c, counter2 = 3 currSlice->next_header = -8888

. . . . . . . .

beginning of loop in image.c, counter2 = 148 currSlice->next_header = -8888
0000(P) 590 7 28 0.0000 0.0000 0.0000 4:2:0 13
0000(P) 592 8 28 0.0000 0.0000 0.0000 4:2:0 17
end of loop in image.c, counter2 = 148 currSlice->next_header = -8888
beginning of loop in image.c, counter2 = 149 currSlice->next_header = -8888
0000(P) 594 9 28 0.0000 0.0000 0.0000 4:2:0 13
0000(P) 596 10 28 0.0000 0.0000 0.0000 4:2:0 16
end of loop in image.c, counter2 = 149 currSlice->next_header = -8888
beginning of loop in image.c, counter2 = 150 currSlice->next_header = -8888
0000(P) 598 11 28 0.0000 0.0000 0.0000 4:2:0 13

. . . . . . . .

I conclude that there is only one call to decode_one_frame, and that
two frames are decoded per pass through the body of the loop in
decode_one_frame.

Here is another argument to support my point: We can search for
next_header in the full program, and we see that it is never set.
Indeed, "grep next_header *" yields

image.c: currSlice->next_header = -8888; // initialized to an impossible value for debugging -- correct value is taken from slice header
image.c: while ((currSlice->next_header != EOS && currSlice->next_header != SOP))

I suspect that to make the code work as expected, currSlice->next_header
would need to be set someplace, for example in exit_macroblock before
returning TRUE.
Additional InformationThe compilation and run was done on an UltraSPARC III+ system,
with Solaris 9, and using a version of the gcc compiler.

I marked the severity as major because this issue gives the
impression that the decoder's architecture is not well thought
out, or that the code is not behaving according to the
architectural plan.

One could suspect that maybe the version of foreman_qcif.264
I used is somehow corrupted. This is probably not the case.
The same behaviour can be seen with many other streams, in
particular with the stream test.264 I created by using
lencod.exe, encoder_baseline.cfg and foreman_part_qcif.yuv
which come with the JM_12.2 package.
TagsNo tags attached.

Relationships

duplicate of 0000035 closedKarsten Suehring decode_one_frame() now decodes all frames! 
parent of 0000189 confirmedKarsten Suehring Crash in JM 16.1 Decoder 
parent of 0000087 confirmedKarsten Suehring decoder quits when partitions are missing (duplicate frame_num in short term ...) 
parent of 0000140 acknowledged Lossy DP videos fail to decode. 
related to 0000250 confirmed "Warning: RTP sequence number discontinuity detected" 
Not all the children of this issue are yet resolved or closed.

Activities

Peter Farkas

2007-07-06 21:07

reporter   ~0000080

Last edited: 2007-07-06 21:11

 I am very sorry, because of a cut and paste error, there are
some inaccuracies in my report. The essence of the bug report
stands as stated, however, there is an error in the details.

I said that "two frames are decoded per pass through the body
of the loop in decode_one_frame". This is not true. Only one
frame is decoded per pass through the body of the loop.

The output (to stdout) I gave in the report is wrong. The
corrected output (which exhibits the problem I reported) is:

. . . . . . . .
in loop in ldecod.c, counter1 = 0
beginning of loop in image.c, counter2 = 0 currSlice->next_header = -8888
end of loop in image.c, counter2 = 0 currSlice->next_header = -8888
beginning of loop in image.c, counter2 = 1 currSlice->next_header = -8888
0000(I) 0 0 28 0.0000 0.0000 0.0000 4:2:0 42
end of loop in image.c, counter2 = 1 currSlice->next_header = -8888
beginning of loop in image.c, counter2 = 2 currSlice->next_header = -8888
0000(P) 2 1 28 0.0000 0.0000 0.0000 4:2:0 17
end of loop in image.c, counter2 = 2 currSlice->next_header = -8888
beginning of loop in image.c, counter2 = 3 currSlice->next_header = -8888
0000(P) 4 2 28 0.0000 0.0000 0.0000 4:2:0 17
end of loop in image.c, counter2 = 3 currSlice->next_header = -8888

. . . . . . . .

beginning of loop in image.c, counter2 = 298 currSlice->next_header = -8888
0000(P) 594 9 28 0.0000 0.0000 0.0000 4:2:0 17
end of loop in image.c, counter2 = 298 currSlice->next_header = -8888
beginning of loop in image.c, counter2 = 299 currSlice->next_header = -8888
0000(P) 596 10 28 0.0000 0.0000 0.0000 4:2:0 17
end of loop in image.c, counter2 = 299 currSlice->next_header = -8888
beginning of loop in image.c, counter2 = 300 currSlice->next_header = -8888
0000(P) 598 11 28 0.0000 0.0000 0.0000 4:2:0 15

. . . . . . . .

Again, sorry about the confusion caused by my error, I am very
embarassed, and I apologize.

The fact still remains that the looping is done not in main in
ldecod.c, but in decode_one_frame in image.c.

Peter Farkas

2007-07-08 00:46

reporter   ~0000081

Here is a change in the code which works in some cases:

In the function exit_macroblock in macroblock.c I changed

  img->num_dec_mb++;

  if (img->num_dec_mb == img->PicSizeInMbs)
  {
    return TRUE;
  }
  // ask for last mb in the slice UVLC
  else
    . . . . . . . .

(at the beginning of the function) to

  img->num_dec_mb++;

  if (img->num_dec_mb == img->PicSizeInMbs)
  {
    Slice *currSlice = img->currentSlice;
    if (currSlice->next_header != EOS)
      currSlice->next_header = SOP;
    return TRUE;
  }
  // ask for last mb in the slice UVLC
  else
    . . . . . . . .

I don't know whether this fix is good in general,
but makes things work as expected in some cases
I tried.

Karsten Suehring

2007-07-08 21:35

administrator   ~0000082

It's known behavior for many (really many) versions now. I planned to restructure the main decoding loop, but didn't have time yet.

Eamon O'Dea

2008-05-29 09:20

reporter   ~0000197

This strange decode_one_frame() logic also appears to be the cause of decoder's failure to decode bit streams in which the frame sizes change.

Since decode_one_frame() never exits

    img->num_dec_mb

never gets reset to zero.

This causes the logic in exit_macroblock() to fail if the frame size changes.

For example, if the initial frame size is CIF (22 x 18 macroblocks) then

    img->PicSizeInMbs

will initially be 396 macroblocks.

When the first frame has been decoded

    img->num_dec_mb

will also be 396 macroblocks

If the frame size changes to VGA (40 x 30 macroblocks) on the second frame then

    img->PicSizeInMbs

will be updated to be 1200 macroblocks.

However

    img->num_dec_mb

will not be reset to zero.

Consequently in exit_macroblock() the logic:

    if (img->num_dec_mb == img->PicSizeInMbs)
    {
        return TRUE;
    }

will return TRUE on the 804th (1200 – 396) macroblock of the second frame. This results in a decode failure.

Karsten Suehring

2008-06-02 11:01

administrator   ~0000204

I already have re-enabled a reset for num_dec_mb in the init_picture() funtion in my development tree.

Based on the standard text there is actually no rule to detect the beginning of a new picture based on the number of macroblocks. This is purely done based on syntax elements in the slice header. So we might be adding some logic here that allow the decoder to process streams that are actually not compliant to the spec (i.e. streams with incorrect header syntax elements).

The situation gets even more complicated when we think about streams with errors...

Issue History

Date Modified Username Field Change
2007-07-06 04:35 Peter Farkas New Issue
2007-07-06 21:07 Peter Farkas Note Added: 0000080
2007-07-06 21:11 Peter Farkas Note Edited: 0000080
2007-07-08 00:46 Peter Farkas Note Added: 0000081
2007-07-08 21:32 Karsten Suehring Relationship added duplicate of 0000035
2007-07-08 21:35 Karsten Suehring Note Added: 0000082
2007-07-08 21:35 Karsten Suehring Status new => acknowledged
2007-08-08 15:04 Karsten Suehring Status acknowledged => assigned
2007-08-08 15:04 Karsten Suehring Assigned To => Karsten Suehring
2008-05-29 09:20 Eamon O'Dea Note Added: 0000197
2008-06-02 11:01 Karsten Suehring Note Added: 0000204
2011-04-06 15:45 Karsten Suehring Relationship added related to 0000250
2011-04-14 15:28 Karsten Suehring Relationship added parent of 0000189
2011-05-18 15:43 Karsten Suehring Target Version => JM 18.1
2011-05-19 13:37 Karsten Suehring Relationship added parent of 0000087
2011-05-21 11:10 Karsten Suehring Relationship added parent of 0000140
2011-11-15 21:54 Karsten Suehring Target Version JM 18.1 => JM 18.2