WLSB problems

[Arne]

During the work with the RoHC library I have found some problems and I only want to support you.

common/wlsb.c
------------------------
281:
  /* change the minimal and maximal values if appropriate */
  if(wlsb->window[entry].value < min)
   min = wlsb->window[entry].value;
  if(wlsb->window[entry].value > max)
   max = wlsb->window[entry].value;

Is there a case where min != max after this lines of code?
However I have exchange the lines 297-303 with the following:
  /* this is a short workaround, if min == max everything is calculated twice */
  if (min != max)
  {
   /* find the minimal number of bits for the lower limit of the interval */
   k1 = g(min, value, wlsb->p, wlsb->bits);
   /* find the minimal number of bits for the upper limit of the interval */
   k2 = g(max, value, wlsb->p, wlsb->bits);

   /* keep the greatest one */
   *bits_nr = (k1 > k2) ? k1 : k2;
  }
  else
  {
   /* find the minimal number of bits for the lower limit of the interval */
   *bits_nr = g(min, value, wlsb->p, wlsb->bits);
  }
This only reduces the count of operations.

common/interval.c
----------------------------
68:
  /* special computation for SN encoding */
  if(k <= 4)
  {
   computed_p = 1;
  }
  else
  {
   computed_p = (1 << (k - 5)) - 1;
  }
It is better to change the if statement to 'if(k <= 5)', because for 5 computed_p is zero.

You handelt the shift operator not consitent. Inside this function the shift operator is set to a positive value for a positve growing window. That works fine. But for a p given directly as parameter for the function is negative (i.a. -1).
This follows is a wrong calculated window. I have fixed this with the following lines (exchanged with lines 80-81):
  /* otherwise: use the p value given as parameter */
// computed_p = p;
  /* the shift defined here is positive, but a shift from the outside is negative for the same direction */
  computed_p = p * (-1);

decomp/feedback.c
------------------------------
At least a short fix for a wrong calculated SN inside the feedback.

85:
  feedback->data[1] = sn & 0xff00 >> 8;
change to
  feedback->data[1] = (sn & 0xff00) >> 8;

I hope that helps you a little bit. Best regards,
Arne

[Didier Barvaux (didier-barvaux)]

For wlsb.c:
 - yes, min may be different of max
 - your optimisation for the min == max case is welcome. Thanks.

For interval.c:
 - the k == 5 case could be handled by a 'else if' branch but not by a 'if(k <= 5)'
    because currently computed_p is 0 for k ==5 not 1
 - I did not understand your analysis of the shift parameter. What's the
   problem with p == -1 ?

For feedback.c: you're right, could be fixed in the 1.2.x and 1.3.x branches too.

Didier

[Arne (poq)]

Hi,

maybe this was a little bit to fast from my side.
The decision for the shift parameter on the compressor side depends on the Profile (for RTP -> 3, others -1). The decompressor initialise the shift value with the lsb_init function, but here always 3 as shift parameter is set. For profiles other than RTP the window on the decompressor side is wrong calculated.

Therefore it was the fastest way to change the value on compressor side from -1 to 1 and than it fits in the window calculated by the shift=3 on the decompressor side. But this is not a good solution. So forgot the my code line, but hold in mind the shift problem.

However I don't understand the calculating of the computed_p value:
  if(k <= 4)
  {
   computed_p = 1;
  }
  else
  {
   computed_p = (1 << (k - 5)) - 1;
  }

As far as I understand this gives the following results:
  bits computed_p
   3 1
   4 1
   5 0
   6 1
I can't find this in RFC 3095, maybe you can give me a hind.

Thanks and sorry for the confusion,
Arne

[Didier Barvaux (didier-barvaux)]

Arne,

> maybe this was a little bit to fast from my side.
> The decision for the shift parameter on the compressor side depends on the Profile
> (for RTP -> 3, others -1). The decompressor initialise the shift value with the lsb_init
> function, but here always 3 as shift parameter is set. For profiles other than RTP the
> window on the decompressor side is wrong calculated.
>
> Therefore it was the fastest way to change the value on compressor side from -1 to 1
> and than it fits in the window >calculated by the shift=3 on the decompressor side. But
> this is not a good solution. So forgot the my code line, but hold in mind the shift problem.
>
> [...]

The shift parameter (also named 'p' in formulas and source code) for the SN field
is not 3 for RTP and -1 for others. I admit the source code is confusing on that part.

RFC 3095, §5.7 says about SN for RTP profile:
    SN: The compressed RTP Sequence Number.
          Compressed with W-LSB. The interpretation intervals, see section
          4.5.1, are defined as follows:
               p = 1 if bits(SN) <= 4
               p = 2^(bits(SN)-5) - 1 if bits(SN) > 4

RFC 3095, §5.11 says about SN for UDP profile:
    The ROHC UDP profile always uses p = -1 when interpreting the SN,
    since there will be no repetitions or reordering of the compressor-
    generated SN. The interpretation interval thus always starts with
    (ref_SN + 1).

So, p = -1 for non-RTP profile and is variable for RTP profile. A shift value of 3 with special
meaning was thus chosen to indicate that the f() function shall adopt the RTP SN behavior.

That's why you see the following code in the f() function when p == 3:
   if(k <= 4)
   {
      computed_p = 1;
    }
   else
   {
      computed_p = (1 << (k - 5)) - 1;
    }

> However I don't understand the calculating of the computed_p value:
> if(k <= 4)
> {
> computed_p = 1;
> }
> else
> {
> computed_p = (1 << (k - 5)) - 1;
> }
>
> As far as I understand this gives the following results:
> bits computed_p
> 3 1
> 4 1
> 5 0
> 6 1
> I can't find this in RFC 3095, maybe you can give me a hind.

This code snippet implements the following algorithm defined for RTP SN
(see RFC 3095, §5.7):
              p = 1 if bits(SN) <= 4
              p = 2^(bits(SN)-5) - 1 if bits(SN) > 4

The special values given to p in order to run special computing of the p parameter
inside the f() function is not a great thing. The code is confusing. That code should
be reworked to be clearer... Maybe define the p variable as an enum with the following
values:
  ROHC_LSB_SHIFT_IP_ID = 0 /* real value for IP-ID */
  ROHC_LSB_SHIFT_RTP_TS = 2 /* special value to compute real value for RTP TS */
  ROHC_LSB_SHIFT_RTP_SN = 3 /* special value to compute real value for RTP SN */
  ROHC_LSB_SHIFT_SN = -1 /* real value for non-RTP SN */

The function calls would be clearer (ie. no more -1, 0, 2 or 3 hardcoded values). The code
of f() function too. What do you think of? Would it be clearer for you?

Regards,
Didier

[Didier Barvaux (didier-barvaux)]

Bug #970762 created for the problem about SN in feedback. Other problems are enhancements, not bugs, so their are lower priority.

[Didier Barvaux (didier-barvaux)]

Won't fix on bugfix-only 1.2.x branch since this ticket lists enhancements, not bugs.

[Didier Barvaux (didier-barvaux)]

Won't fix on bugfix-only 1.3.x branch since this ticket lists enhancements, not bugs.

[Didier Barvaux (didier-barvaux)]

Fixed in trunk, see http://bazaar.launchpad.net/~didier-barvaux/rohc/main/revision/334 and http://bazaar.launchpad.net/~didier-barvaux/rohc/main/revision/335