Vladimir, help with JPEG please

Vladimir, help with JPEG please

I have ran into an issue with IPP JPEG I cannot sort out on my own.

Namely I am experimenting with an encoder I wrote which should have lossless as well as lossy mode.

I made it so that if I pass quality 100, it performs just DCT with no quantization using ippiDCT8x8Fwd_16s_C1R() and I perform level shift beforehand. Mind you I work with 16-bit data (12 bits are actually important) and so far I am just storing the result of the DCT to see if I can reverse the operation.

When the quality is less than 100 then I call ippiDCTQuantFwd8x8LS_JPEG_16u16s_C1R() without performing level shift on my own and I pass it properly prepared quantization table.

Calling ippiDCTQuantInv8x8LS_JPEG_16s16u_C1R() works fine, but with ippiDCT8x8Inv_16s_C1R() I get complete garbage.

I checked the documentation and I was astonished to find that ippiDCT8x8Inv_16s_C1R() has this warning:

CAUTION: Source data for 16s flavors must be the result of the forward discrete cosine transform of data from the range [-256, 255], they can not be arbitrary data from the range [-32768, 32767].

Damn!!!

Function documentation for ippiDCT8x8Fwd_16s_C1R() does not mention anywhere that its output is limited to 9 bits!!!

So why do you advertise 12-bit or even 16-bit support for JPEG when it simply doesn't work?!?

What is the rationale behind "regular DCT without bundled quantization and level shift doesn't support more than 9 bits" and is there any way around this nonsense?

I am sorry if I sound pissed off but it is because I put a lot of time into this and I need help really quick like yesterday.

I find IPP documentation sorely lacking on this subject.

-- Regards, Igor Levicki If you find my post helpfull, please rate it and/or select it as a best answer where applies. Thank you.
17 posts / 0 new
Last post
For more complete information about compiler optimizations, see our Optimization Notice.

Hi Igor,

The JPEG standard supports several coding processes and IPP JPEG codec (which is part of JPEGView sample) do supports some of them:

1. Lossy, DCT based, huffman entropy coding for 8-bit input data baseline and progressive processes

2. Lossy, DCT based, huffman entropy coding for 12-bit input data, extended baseline process

3. Lossless, huffman entropy coding for the input data in range 2..16 bits inclusive.

The function ippiDCT8x8Inv_16s_C1R() (declared in ippi.h) was implemented mainly for video coding purposes and supports input data in 8-bits precision.

For using in JPEG codec we have extended set of DCT functions with ippiDCTQuantFwd8x8LS_JPEG_16u16s_C1R() and ippiDCTQuantInv8x8LS_JPEG_16s16u_C1R() to support extended baseline JPEG process with input data in 12-bit range. It is not appropriate to perform forward DCT transform on input data with extended range and then try to reconstruct coefficients with inverse DCT function which developed for 8-bit data range. You need to useappropriate functions in pair.

We do advertise support for extended data range in IPP JPEG codec because we have implemented and tested it in JPEGView sample. You can easely prove that fact with JPEGView sample. It is able to open PNM or JPEG2000 files with up to 16 bit per color channel data and then you can encode this into JPEG with "Save As" dialog. Note, to be able to encode 12-bit extended baseline JPEG files your input data should be in 12-bit range (sample do not support truncating 16-bit data to 12-bit range to encode them in extended baseline mode).

We are sorry for possible not perfect documentation, you know it is not easy to document 9000+ functions in IPP with all the details, but fortunately we can timely help you on this forum.

BTW, why do you need to re-implement JPEG codec? What prevent you from using JPEG codec(from JPEGView sample) directly?

(I'm assuming you use IPP and sample v5.2)

Regards,
Vladimir

why do you need to re-implement JPEG codec?

Lets just say that I am trying to "roll my own" compression format for 12-bit data which should have both lossless and lossy mode and that I have some ideas on how to transform DCT coefficients to improve compression ratio.

If I understand correctly, DCT and IDCT (without quantization or resampling) are lossless operations, right?

What I want is to be able to perform DCT/IDCT on 8x8 block with 12-bit data precision without quantization.

So far I wasn't able to do it using IPP because only combined DCT and quantization functions support more than 8-bit precision.

Moreover, IPP itself lacks proper functions for initialization of Ipp32f quantization and dequantization tables so one either has to write those functions or borrow them from your sample code.

Since those functions are needed for 12-bit support and they are not part of the library, I fail to understand how is it that you can claim that IPP has complete 12-bit support built-in?

As for documentation, who needs 5,000+ functions if he can't make it out what the proper arguments and calling order are? At least for me Quality beats Quantity here by a large margin.

-- Regards, Igor Levicki If you find my post helpfull, please rate it and/or select it as a best answer where applies. Thank you.

Igor,

we claim that IPP JPEG codec supports lossy 8-bit, lossy 12-bit and lossless 2..16 bit compression processes compatible withISO/IEC 10918 (thereis number of limitations listed in sample's readme.htm). I do not know how it correlates with your own 12-bit compression format.

Theoretically, DCT/IDCT operation is lossless but on practice rounding errors could affect transform precision, especially if you are limited by 16-bits. But there is ippiDCTFwd_32f_C1R function which work with single precision floating point data.

There is no reason to add initialization functions into the library as they do not take the most computational time. It is fine to use those from sample or to write your own if you want.

Regards,
Vladimir

Vladimir, I am just saying that I miss ippiDCTFwd8x8LS_JPEG_16u16s_C1R() function.

Furthermore, ippiDCT8x8Fwd_16s_C1R() and ippiDCT8x8Inv_16s_C1R() have a misleading name — 16s implies that they can deal with full -32768, 32767 range or at least with 12 bits of precision while in fact they work with -256, 255 which is only 9 bits of precision.

As for ippiDCTFwd_32f_C1R():

  • It would require two data conversions (Ipp16s->Ipp32f and Ipp32f->Ipp16s)
  • It would require three times more memory (w * h * sizeof(Ipp16s) + w * h * sizeof(Ipp32f))
  • It performs DCT of the whole image instead of 8x8 blocks
  • It would probably be slower than the 8x8 integer based DCT

Initialization functions are needed for the sake of implementation completeness and not for performance reasons. That is exactly why they would be very easy to add, you won't waste any time on their optimization.

By the way, documentation is lacking clear explanations which of those functions expect quantization tables in normal, and which ones in zig-zag order.

Oh and the sample code in ippiQuantFwdRawTableInit_JPEG_16u() has a possible divide by zero bug, you need to add something like this:

	Quality = __max(1, __min(100, Quality));

Instead of just:

  if(quality > 100)
    quality = 100;

Whoever wrote that sample code, please tell them not to be so lame — if is a language keyword, not a function call so there should be a space between if and (. Furthermore, tell them that if they prefer small indentation, they should at least select "Keep tabs" option in the editor instead of hard-coding spaces into the code. Also, their use of curly braces is inconsistent and it is impairing code readability. Consider this:

  if(quality < 50)
    quality = 5000 / quality;
  else
    quality = 200 - (quality * 2);

And this:

    if(val < 1)
    {
      raw[i] = (Ipp16u)1;
    }
    else if(val > 65535)
    {
      raw[i] = (Ipp16u)65535;
    }
    else
    {
      raw[i] = (Ipp16u)val;
    }

Both snippets come from the same function! It would look much better like this:

	if (val < 1) {
		raw[i] = (Ipp16u)1;
	} else if (val > 65535) {
		raw[i] = (Ipp16u)65535;
	} else {
		raw[i] = (Ipp16u)val;
	}

Not to mention that it can be shortened to a single row:

	raw[i] = __max(1, __min(65535, val);
-- Regards, Igor Levicki If you find my post helpfull, please rate it and/or select it as a best answer where applies. Thank you.

Igor,

I would refer you to the beginning of IPP Image Processing manual, chapter Intel Integrated Performance Primitives Concepts, to not be misleading by data type descriptors in IPP function names. And you already discovered caution about allowable data range for 16-bit DCT functions, is not it?

You are correct, the performance of DCT function for Ipp32f data type is lower then performance of ippiDCT8x8Fwd_16s_C1R() function. Please be aware that ippiDCTFwd8x8LS_JPEG_16u16s_C1R() function internally use single precision floating point calculations (and integer to float and vice verse transform accordingly). This is because of internal calculation in DCT require more than 16-bit precision for the case of 12-bit input data.

Thanks for pointing to possible divide by zero bug, we will verify that. I think that guard from this is placed on this function caller, but I will agree with you it is better to check arguments inside of function.

The coding style of IPP samples is dictated by coding guideline document accepted in our team. It might be different from your preferable coding style but it at least keeps the text in consistent way.

Again, thank you for your review, I think it will help us to improve that sample and documentation in future versions.

Regards,
Vladimir

Please be aware that ippiDCTFwd8x8LS_JPEG_16u16s_C1R() function internally use single precision floating point calculations (and integer to float and vice verse transform accordingly). This is because of internal calculation in DCT require more than 16-bit precision for the case of 12-bit input data.

Yes, I am aware of that although you have probably meant to say ippiDCTQuantFwd8x8LS_JPEG_16u16s_C1R() because ippiDCTFwd8x8LS_JPEG_16u16s_C1R() still doesn't exist (I just asked for it).

The coding style of IPP samples is dictated by coding guideline document accepted in our team. It might be different from your preferable coding style but it at least keeps the text in consistent way.

Vladimir, I have just shown you that it is not consistent — sometimes if/else/endif blocks have curly braces and sometimes they do not.

Furthermore, I am not imposing my coding style on you, I am just pointing out two issues I have with your (corporate) coding style:

  1. Hard-coded spaces
  2. No space between if and (

I understand that different people prefer different number of spaces for indentation. For example, you have your TAB set to two spaces and that is your (corporate) standard. I do not have a problem with the amount of indentation, but with the way you are accomplishing it.

My rationale:
Hardcoded spaces can't be easily replaced to match someone else's indentation liking. Also, there is a confusion about what is hard and what is soft, people call TABS hard and they call spaces soft. Actually, it is the other way around. Why? Because when you replace TABS with spaces you hardcode the amount of spaces in the indentation! If you keep TABS your editor will replace them with adequate number of spaces on display.

The whole point of indentation is to make the source code more readable for humans, because compiler certainly doesn't care about the whitespace. Two-space indent on say 1600x1200 20" TFT doesn't make it more readable, and having to process all those source files to change indentation to your liking is very tedious and error prone task.

So I am just saying that by keeping TABs instead of spaces that problem can be avoided. Is that too much to ask?

As for #2, as I said, if is a language keyword, not a function call so in my opinion it shouldn't be written the same way you write a function call.

The most reasonable coding style in my opinion is Linux Kernel Coding Style — I strongly suggest you to check it out, it is also a funny read.

-- Regards, Igor Levicki If you find my post helpfull, please rate it and/or select it as a best answer where applies. Thank you.

Igor,

coding rule we use says that it is better to use curly braces for multi-line blocks and it is allowed to not use braces for single line block. So, what you pointed outis not necessary showlack of consistancy.

The main issue with TAB symbols is that your text might look differently on various editors. Using hard coded SPACEs allow to avoid that.

I'm aware of several different coding styles, including one you pointed out, but the fact is that we have choose one andwe follow it.

Regards,
Vladimir

coding rule we use says that it is better to use curly braces for multi-line blocks and it is allowed to not use braces for single line block. So, what you pointed out is not necessary show lack of consistancy.

In this case it is lack of consistency, because both if/else blocks had only single lines in them.

The main issue with TAB symbols is that your text might look differently on various editors. Using hard coded SPACEs allow to avoid that.

Jesus... I would expect developers to use logic and rational thinking but that seems not to be the case today. You have three possible solutions for the issue you are describing:

  1. Set all editors you use to have the same TAB size and keep TABs
  2. Stick with only one editor, set your prefered TAB size and keep TABs
  3. Use whatever editor you feel like using at the moment in an inconsistent manner (with different settings), and piss off everyone else by hardcoding spaces instead of keeping TABs just because it is too hard for you to be consistent

Why are you intentionally chosing the worst, most intolerant and most incompatible option (#3)?

Just so you know, in my opinion this is an attitude problem, and not a conflict between our coding styles.

If I write some code and send it to you, you will instantly see it with your prefered level of indentation but if you send me some code, I will have to pre-process it in order to get the same result. So think again, which side is intolerant when it comes to different coding styles?

-- Regards, Igor Levicki If you find my post helpfull, please rate it and/or select it as a best answer where applies. Thank you.

Igor,

you might not consider as an issue the necessity to tune editors on each platform you are working on to comfortable settings but we do.

Your approach probably should work in case you use only TABs, but in the reality, the many of source files which use mix of TAB and SPACEs have an indentation issue when you use other thenoriginal TAB settings.

Vladimir

but in the reality, the many of source files which use mix of TAB and SPACEs have an indentation issue when you use other then original TAB settings.

That is exactly the reason why you should use only TABs. It is easy to change TAB width, but not so easy to change the number of SPACEs. :)

-- Regards, Igor Levicki If you find my post helpfull, please rate it and/or select it as a best answer where applies. Thank you.

Or only SPACEs...

As you can't use ONLY TABs in whole text the only option left is to useSPACEs.

Vladimir

I forgot to reply to this:

coding rule we use says that it is better to use curly braces for multi-line blocks and it is allowed to not use braces for single line block. So, what you pointed out is not necessary show lack of consistancy.

It is inconsistent. Here you don't have braces and blocks are single lines:

if(quality < 50)
    quality = 5000 / quality;
  else
    quality = 200 - (quality * 2);

And here you have braces, and blocks are again single lines:

    if(val < 1)
    {
      raw[i] = (Ipp16u)1;
    }
    else if(val > 65535)
    {
      raw[i] = (Ipp16u)65535;
    }
    else
    {
      raw[i] = (Ipp16u)val;
    }

Also, not using braces for single line blocks exposes you to the risk of someone doing this:


#include 
#include 

#define scale_and_clamp(x, clamp) x *= 2; if (x > clamp) x = clamp;

int func(int c)
{
	if (c < 50)
		scale_and_clamp(c, 100)
	else
		c -= 30;

	return c;
}

int main(int argc, char *argv[])
{
	if (--argc == 0) {
		printf("usage : %s 
", argv[0]);
		return 1;
	}
	int i = atoi(argv[1]);
	printf("%d
", func(i));
	return 0;
}

Run this short demo program and see if you are getting results you would have expected to get.

-- Regards, Igor Levicki If you find my post helpfull, please rate it and/or select it as a best answer where applies. Thank you.

Thanks, the rule we are following allows us to not use brackets for single line but do not required that. It is up to developer to decide when it is better to wrap single line with brackets and when not.

And your example is more about another rule - code inside macro definition should be bounded by brackets.

Regards,
Vladimir

And your example is more about another rule - code inside macro definition should be bounded by brackets.

I disagree. Bounding macro code with braces changes scope. It tempts you to declare temporary variables inside a macro block which is bad. Those variables can hide variables with the same name in the surrounding code and you can get weird results like in this example:

#define scale_and_clamp(x, clamp)	
{					
	int c = 2;			
	x *= c;				
	if (x > clamp)			
		x = clamp;		
}
-- Regards, Igor Levicki If you find my post helpfull, please rate it and/or select it as a best answer where applies. Thank you.

Igor,

yes there are many places were you should be careful when write program. Regarding your sample I see no reason to declare temporary variable c, so I do not know why you think that something tempts you to do that.

Regards,
Vladimir

There is no real need in the current sample. I just wanted to say that when you have a block, you are tempted to use variables because you know their scope is limited by the block.

-- Regards, Igor Levicki If you find my post helpfull, please rate it and/or select it as a best answer where applies. Thank you.

Login to leave a comment.