Opened 9 years ago

Closed 9 years ago

#840 closed defect (fixed)

Slices incorrectly encoded when wavefront is enabled

Reported by: bheng Owned by:
Priority: minor Milestone: HM-9.1
Component: HM Version: HM-9.0
Keywords: Cc: fbossen, ksuehring, davidf, jct-vc@…

Description

During encoding, the derivation of SliceCurEndCUAddr can be inconsistent between the compressSlice() and encodeSlice() functions when wavefront is used with slices. This causes the same CTB to be re-encoded into more than one slice.

I believe the attached patch should fix the issue.

Attachments (4)

wavefront_slice_bug.patch (889 bytes) - added by bheng 9 years ago.
wpp_840_slice_length.patch (1.4 KB) - added by stworrall 9 years ago.
Patch for original ticket 840 patch
slice_restriction.patch (2.1 KB) - added by bheng 9 years ago.
dependent_slice_restriction.patch (1.7 KB) - added by bheng 9 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 9 years ago by DefaultCC Plugin

  • Cc fbossen ksuehring davidf jct-vc@… added

Changed 9 years ago by bheng

comment:2 Changed 9 years ago by ksuehring

  • Resolution set to fixed
  • Status changed from new to closed

the patch has been applied in r3062

Changed 9 years ago by stworrall

Patch for original ticket 840 patch

comment:3 Changed 9 years ago by stworrall

  • Resolution fixed deleted
  • Status changed from closed to reopened

While this patch correctly identifies the source of a problem for WPP and slices, it also introduces a constraint on slice boundaries for WPP that is not present in the draft text.

If a slice starts in the middle of a row, then the constraint forces the slice to end at the end of the current LCU row. This does not seem to be required by the text.

The intention of the patch is to align the results of the compressSlice() and encodeSlice() functions, which is necessary to fix the bug. But I think a better solution would be to remove the constraint for both cases, rather than enforce the constraint for both.

I have attached a patch that removes the constraints. The patch was derived from modifications to http://hevc.hhi.fraunhofer.de/svn/svn_HEVCSoftware/branches/HM-9.0-dev@3071

comment:4 Changed 9 years ago by semihese

It is required by the text!

Please check the semantics for entropy_coding_sync_enabled_flag.

In fact if a slice starts in the middle of an LCU row, it must terminate before the end of the current LCU row. The same restriction also applies to slice segments.

Therefore it would be better to keep the constraint in the software.

comment:5 Changed 9 years ago by stworrall

But I don't think that this is what the existing software constraint does. The existing constraint forces the second slice in an LCU row to finish at the end of the current row. The text says that the slice shall finish in the same row. In other words, it should be possible to have many slices in a single row, not just two.

To fix the original bug (encoding CTUs twice) we can either remove the constraint altogether, or fix the constraint so that it allows valid cases to be encoded. An extra conditional check to see if the slice crosses over to the next row should also fix the problem.

Original code:

if (pcSlice->getPPS()->getNumSubstreams() > 1 && (uiStartCUAddrSlice % (rpcPic->getFrameWidthInCU()*rpcPic->getNumPartInCU()) != 0) ) 
{ 
  uiBoundingCUAddrSlice = uiStartCUAddrSlice - (uiStartCUAddrSlice % (rpcPic->getFrameWidthInCU()*rpcPic->getNumPartInCU())) + (rpcPic->getFrameWidthInCU()*rpcPic->getNumPartInCU()); 
}

With an extra condition to check if the slice boundary is in the next row:

if (pcSlice->getPPS()->getNumSubstreams() > 1 && (uiStartCUAddrSlice % (rpcPic->getFrameWidthInCU()*rpcPic->getNumPartInCU()) != 0)
    && ( uiStartCUAddrSlice/rpcPic->getFrameWidthInCU() != uiBoundingCUAddrSlice/rpcPic->getFrameWidthInCU() ) ) 
{ 
  uiBoundingCUAddrSlice = uiStartCUAddrSlice - (uiStartCUAddrSlice % (rpcPic->getFrameWidthInCU()*rpcPic->getNumPartInCU())) + (rpcPic->getFrameWidthInCU()*rpcPic->getNumPartInCU()); 
}

comment:6 Changed 9 years ago by mcoban

It is allowed to have many slices in a single row. Steve's fix with extra conditional check to see if the slice crosses over to the next row to terminate a slice should fix the problem and make the software and text consistent.

comment:7 follow-up: Changed 9 years ago by bheng

I wouldn't recommend removing the constraint altogether since that would allow the encoder to create illegal streams that violate the restrictions in the text. However, the extra condition suggested above wouldn't work correctly either since uiBoundingCUAddrSlice and getFrameWidthInCU() aren't on the same scale (missing factor of getNumPartInCU()).

But I agree that the current code is enforcing a condition stronger than the one specified in the text.

I have attached an additional patch that just adds a MIN to prevent the encoder from changing the uiBoundingCUAddrSlice unless it is actually necessary. I believe that would fix the issue.

Changed 9 years ago by bheng

comment:8 in reply to: ↑ 7 Changed 9 years ago by stworrall

Replying to bheng:

I have attached an additional patch that just adds a MIN to prevent the encoder from changing the uiBoundingCUAddrSlice unless it is actually necessary. I believe that would fix the issue.

This looks like a good fix. It would be useful to include this fix in HM-9.1, as it correctly prevents the encoder from generating a non-conforming WPP bitstream.

comment:9 Changed 9 years ago by ksuehring

I have applied slice_restriction.patch in r3085. The same restrictions apply for dependent slices (or dependent slice segments as they are defined in the text now). Can we apply the same restriction some lines below in the dependent slice code?

comment:10 Changed 9 years ago by bheng

Thanks Karsten. Yes, it would be good to fix this for dependent slice segments as well. I have added the additional restriction in the attached patch (dependent_slice_restriction.patch).

Changed 9 years ago by bheng

comment:11 Changed 9 years ago by ksuehring

  • Resolution set to fixed
  • Status changed from reopened to closed

Thank you. I have applied the patch in r3091

Note: See TracTickets for help on using tickets.

This list contains all users that will be notified about changes made to this ticket.

These roles will be notified: Reporter, Owner, Subscriber, Participant

  • Brian Heng(Reporter, Participant)
  • David Flynn(Subscriber)
  • Frank Bossen(Subscriber)
  • jct-vc@…(Subscriber)
  • karl.sharman@…(Always)
  • Karsten Suehring(Subscriber, Participant, Always)
  • Muhammed Coban(Participant)
  • Stewart Worrall(Participant)