Opened 9 years ago

Closed 9 years ago

#1358 closed defect (fixed)

pps_extension_flag misleading syntax

Reported by: kolya Owned by:
Priority: minor Milestone: HM-16.3
Component: HM Version: HM-16.2
Keywords: Cc: ksuehring, davidf, karlsharman, jct-vc@…

Description

In JCTVC-Q1003 document there is

pps_extension_flag
if( pps_extension_flag )

while( more_rbsp_data( ) )

pps_extension_data_flag

rbsp_trailing_bits( )

while in JCTVC-Q1005 there is

pps_extension_present_flag
if( pps_extension_present_flag ) {

pps_range_extensions_flag
pps_extension_7bits

}

while in HM-dev s\w there is

READ_FLAG( uiCode, "pps_extension_present_flag");
if (uiCode)
{

Bool pps_extension_flags[NUM_PPS_EXTENSION_FLAGS];
for(Int i=0; i<NUM_PPS_EXTENSION_FLAGS; i++)
{

READ_FLAG( uiCode, "pps_extension_flag[]" );
pps_extension_flags[i] = uiCode!=0;

}

It seems that there is some misleading naming of flags, at least in s\w.

Attachments (2)

pps(sps)_extension_flag.patch (2.0 KB) - added by kolya 9 years ago.
pps(sps)_extension_flag_encoder.patch (3.0 KB) - added by kolya 9 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 9 years ago by DefaultCC Plugin

  • Cc ksuehring davidf karlsharman jct-vc@… added

comment:2 Changed 9 years ago by ksuehring

Please provide patches relative to HM-dev.

Changed 9 years ago by kolya

Changed 9 years ago by kolya

comment:3 Changed 9 years ago by kolya

Provided patches of minimal relevant changes. The reasoning to exclude "almost-the-same" naming from the reference s\w.

Version 0, edited 9 years ago by kolya (next)

comment:4 Changed 9 years ago by karlsharman

Please check against R1013_v6 (latest version 2 document) for
"...extension_flag" and "...extension_present_flag".

comment:5 Changed 9 years ago by kolya

Yes, there is one more flag is split from 8-bit chunk. However, my concern was that "pps_extension_flags" encountered in s\w were called instead of "pps_extension_6bits". For analysis among several hundred of syntax names the search for the match is painful, while fixing the s\w once is not of that.

comment:6 Changed 9 years ago by ksuehring

Based on the patches I understand what you mean.

Usually I prefer to have HLS names aligned to the code, especially the trace strings. But in this case the current code looks much more flexible with the intention of the first eight extension bits. The multi-layer extension (as seen in the syntax in R1013_v6) can easily be added in another "case" statement. We need to support only the RExt extension in HM, but SHM and the MV/3D software can easily extend the structure here (maybe even SCC with another extension).

I don't like the renaming of the array to pps_extension_Xbits, because it does not represent the standard text either. The only option that I would consider is to follow the syntax exactly and parse the already defined existence flags one by one and then the remainder. But with mentioned better flexibility, I would prefer to keep the code as is.

Last edited 9 years ago by ksuehring (previous) (diff)

comment:7 Changed 9 years ago by kolya

Then, my proposal boils down to move naming to R1013_v6 case, and udpate naming along text fixing; 4 times per year. Unlike PTL section, where 4 different syntax elements are named as the same, the extensions marking part is twice shorter and should not take too much efforts.

comment:8 Changed 9 years ago by kolya

Could you please clarify the following (and related if something is):

JCTVC-R1013_v6 contains

vps_base_layer_internal_flag
vps_base_layer_available_flag

But the code has

READ_CODE( 2, uiCode, "vps_reserved_three_2bits" ); assert(uiCode == 3);

When it is supposed to change the code to match?

comment:9 Changed 9 years ago by ksuehring

  • Milestone set to HM-16.3

comment:10 Changed 9 years ago by karlsharman

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

Fixed in r4252. A new ticket (#1365) has been created to highlight that some renaming of variables and functions is needed in order to be consistent with the text.

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

  • David Flynn(Subscriber)
  • jct-vc@…(Subscriber)
  • Karl Sharman(Subscriber, Participant)
  • karl.sharman@…(Always)
  • Karsten Suehring(Subscriber, Participant, Always)
  • Nikolay Shlyakhov(Reporter, Participant)