Skip to main content
Fix casing of record, add performance statement
Source Link
Thomas Weller
  • 1.7k
  • 14
  • 25

IMG is not a PascalCase class name as it should be in .NET. Why not HiddenDataImage or something similar?

Likewise, Data should be data (camelCase)Likewise, Data should be data (camelCase).

That statement was incorrect. Record types use PascalCase [MSDN]:

When writing positional records, use pascal casing for parameters as they're the public properties of the record.

int Size = Data.Length >> 2; should not be a bit-shift operation. There is no bit manipulation in this class. This should probably be int Size = Data.Length / (BitsPerPixel/8);, where BitsPerPixel is 32. Let the compiler optimize that into a bit shift operation.

Any performance arguments are not valid to me in this code base. If you have the time to hide a byte array in a GDI+ Bitmap, performance is certainly not your primary problem. Benchmarks on my PC could not find a difference between the bitshift and the division. The JITed assembly code can be seen on Sharplap.

The throw new NotSupportedException(nameof(bitmap)) is not helpful. That exception should explain that only 32 bit ARGB images are supported.

Note that the byte[] from Load() is not identical to the stored one. Your bitmap will be a square. Thus, when you originally store 17 bytes, you get back 25 bytes. And those extra bytes may contain garbage.

Is there any cases where padding would be an issue?

Padding in a Bitmap is called Stride. As Microsoft says:

The stride is the width of a single row of pixels (a scan line), rounded up to a four-byte boundary.

So, in your case, as long as you stick with 32 bit ARGB, stride and width are identical. But you have the square padding issue. A solution might be to store the array size in the first 32 bits of the image, adding that as an overhead.

IMG is not a PascalCase class name as it should be in .NET. Why not HiddenDataImage or something similar?

Likewise, Data should be data (camelCase).

int Size = Data.Length >> 2; should not be a bit-shift operation. There is no bit manipulation in this class. This should probably be int Size = Data.Length / (BitsPerPixel/8);, where BitsPerPixel is 32. Let the compiler optimize that into a bit shift operation.

The throw new NotSupportedException(nameof(bitmap)) is not helpful. That exception should explain that only 32 bit ARGB images are supported.

Note that the byte[] from Load() is not identical to the stored one. Your bitmap will be a square. Thus, when you originally store 17 bytes, you get back 25 bytes. And those extra bytes may contain garbage.

Is there any cases where padding would be an issue?

Padding in a Bitmap is called Stride. As Microsoft says:

The stride is the width of a single row of pixels (a scan line), rounded up to a four-byte boundary.

So, in your case, as long as you stick with 32 bit ARGB, stride and width are identical. But you have the square padding issue. A solution might be to store the array size in the first 32 bits of the image, adding that as an overhead.

IMG is not a PascalCase class name as it should be in .NET. Why not HiddenDataImage or something similar?

Likewise, Data should be data (camelCase).

That statement was incorrect. Record types use PascalCase [MSDN]:

When writing positional records, use pascal casing for parameters as they're the public properties of the record.

int Size = Data.Length >> 2; should not be a bit-shift operation. There is no bit manipulation in this class. This should probably be int Size = Data.Length / (BitsPerPixel/8);, where BitsPerPixel is 32. Let the compiler optimize that into a bit shift operation.

Any performance arguments are not valid to me in this code base. If you have the time to hide a byte array in a GDI+ Bitmap, performance is certainly not your primary problem. Benchmarks on my PC could not find a difference between the bitshift and the division. The JITed assembly code can be seen on Sharplap.

The throw new NotSupportedException(nameof(bitmap)) is not helpful. That exception should explain that only 32 bit ARGB images are supported.

Note that the byte[] from Load() is not identical to the stored one. Your bitmap will be a square. Thus, when you originally store 17 bytes, you get back 25 bytes. And those extra bytes may contain garbage.

Is there any cases where padding would be an issue?

Padding in a Bitmap is called Stride. As Microsoft says:

The stride is the width of a single row of pixels (a scan line), rounded up to a four-byte boundary.

So, in your case, as long as you stick with 32 bit ARGB, stride and width are identical. But you have the square padding issue. A solution might be to store the array size in the first 32 bits of the image, adding that as an overhead.

deleted 305 characters in body
Source Link
Thomas Weller
  • 1.7k
  • 14
  • 25

IMG is not a PascalCase class name as it should be in .NET. Why not HiddenDataImage or something similar?

Likewise, Data should be data (camelCase).

int Size = Data.Length >> 2; should not be a bit-shift operation. There is no bit manipulation in this class. This should probably be int Size = Data.Length / (BitsPerPixel/8);, where BitsPerPixel is 32. Let the compiler optimize that into a bit shift operation.

The throw new NotSupportedException(nameof(bitmap)) is not helpful. That exception should explain that only 32 bit ARGB images are supported.

IMHO, there's a bug retrieving the data inNote that the Load() method. var size = bitData.Height * bitData.Stride;byte[] does not acknowledge that there are 32 bits per pixel. This should befrom var size = bitData.Height * bitData.Stride * Load(BitsPerPixel/8);. Write unit tests to confirm that your implementation works well.

Also note that the byte[] is not identical to the stored one. Your bitmap will be a square. Thus, when you originally store 17 bytes, you get back 25 bytes. And those extra bytes may contain garbage.

Is there any cases where padding would be an issue?

Padding in a Bitmap is called Stride. As Microsoft says:

The stride is the width of a single row of pixels (a scan line), rounded up to a four-byte boundary.

So, in your case, as long as you stick with 32 bit ARGB, stride and width are identical. But you have the square padding issue. A solution might be to store the array size in the first 32 bits of the image, adding that as an overhead.

IMG is not a PascalCase class name as it should be in .NET. Why not HiddenDataImage or something similar?

Likewise, Data should be data (camelCase).

int Size = Data.Length >> 2; should not be a bit-shift operation. There is no bit manipulation in this class. This should probably be int Size = Data.Length / (BitsPerPixel/8);, where BitsPerPixel is 32. Let the compiler optimize that into a bit shift operation.

The throw new NotSupportedException(nameof(bitmap)) is not helpful. That exception should explain that only 32 bit ARGB images are supported.

IMHO, there's a bug retrieving the data in the Load() method. var size = bitData.Height * bitData.Stride; does not acknowledge that there are 32 bits per pixel. This should be var size = bitData.Height * bitData.Stride * (BitsPerPixel/8);. Write unit tests to confirm that your implementation works well.

Also note that the byte[] is not identical to the stored one. Your bitmap will be a square. Thus, when you originally store 17 bytes, you get back 25 bytes. And those extra bytes may contain garbage.

Is there any cases where padding would be an issue?

Padding in a Bitmap is called Stride. As Microsoft says:

The stride is the width of a single row of pixels (a scan line), rounded up to a four-byte boundary.

So, in your case, as long as you stick with 32 bit ARGB, stride and width are identical. But you have the square padding issue. A solution might be to store the array size in the first 32 bits of the image, adding that as an overhead.

IMG is not a PascalCase class name as it should be in .NET. Why not HiddenDataImage or something similar?

Likewise, Data should be data (camelCase).

int Size = Data.Length >> 2; should not be a bit-shift operation. There is no bit manipulation in this class. This should probably be int Size = Data.Length / (BitsPerPixel/8);, where BitsPerPixel is 32. Let the compiler optimize that into a bit shift operation.

The throw new NotSupportedException(nameof(bitmap)) is not helpful. That exception should explain that only 32 bit ARGB images are supported.

Note that the byte[] from Load() is not identical to the stored one. Your bitmap will be a square. Thus, when you originally store 17 bytes, you get back 25 bytes. And those extra bytes may contain garbage.

Is there any cases where padding would be an issue?

Padding in a Bitmap is called Stride. As Microsoft says:

The stride is the width of a single row of pixels (a scan line), rounded up to a four-byte boundary.

So, in your case, as long as you stick with 32 bit ARGB, stride and width are identical. But you have the square padding issue. A solution might be to store the array size in the first 32 bits of the image, adding that as an overhead.

added 196 characters in body
Source Link
Thomas Weller
  • 1.7k
  • 14
  • 25

IMG is not a PascalCase class name as it should be in .NET. Why not HiddenDataImage or something similar?

Likewise, Data should be data (camelCase).

int Size = Data.Length >> 2; should not be a bit-shift operation. There is no bit manipulation in this class. This should probably be int Size = Data.Length / (BitsPerPixel/8);, where BitsPerPixel is 32. Let the compiler optimize that into a bit shift operation.

The throw new NotSupportedException(nameof(bitmap)) is not helpful. That exception should explain that only 32 bit ARGB images are supported.

IMHO, there's a bug retrieving the data in the Load() method. var size = bitData.Height * bitData.Stride; does not acknowledge that there are 32 bits per pixel. This should be var size = bitData.Height * bitData.Stride * (BitsPerPixel/8);. Write unit tests to confirm that your implementation works well.

Also note that the byte[] is not identical to the stored one. Your bitmap will be a square. Thus, when you originally store 17 bytes, you get back 25 bytes. And those extra bytes may contain garbage.

Is there any cases where padding would be an issue?

Padding in a Bitmap is called Stride. As Microsoft says:

The stride is the width of a single row of pixels (a scan line), rounded up to a four-byte boundary.

So, in your case, as long as you stick with 32 bit ARGB, stride and width are identical. But you have the square padding issue. A solution might be to store the array size in the first 32 bits of the image, adding that as an overhead.

IMG is not a PascalCase class name as it should be in .NET. Why not HiddenDataImage or something similar?

int Size = Data.Length >> 2; should not be a bit-shift operation. There is no bit manipulation in this class. This should probably be int Size = Data.Length / (BitsPerPixel/8);, where BitsPerPixel is 32. Let the compiler optimize that into a bit shift operation.

IMHO, there's a bug retrieving the data in the Load() method. var size = bitData.Height * bitData.Stride; does not acknowledge that there are 32 bits per pixel. This should be var size = bitData.Height * bitData.Stride * (BitsPerPixel/8);. Write unit tests to confirm that your implementation works well.

Also note that the byte[] is not identical to the stored one. Your bitmap will be a square. Thus, when you originally store 17 bytes, you get back 25 bytes. And those extra bytes may contain garbage.

Is there any cases where padding would be an issue?

Padding in a Bitmap is called Stride. As Microsoft says:

The stride is the width of a single row of pixels (a scan line), rounded up to a four-byte boundary.

So, in your case, as long as you stick with 32 bit ARGB, stride and width are identical. But you have the square padding issue. A solution might be to store the array size in the first 32 bits of the image, adding that as an overhead.

IMG is not a PascalCase class name as it should be in .NET. Why not HiddenDataImage or something similar?

Likewise, Data should be data (camelCase).

int Size = Data.Length >> 2; should not be a bit-shift operation. There is no bit manipulation in this class. This should probably be int Size = Data.Length / (BitsPerPixel/8);, where BitsPerPixel is 32. Let the compiler optimize that into a bit shift operation.

The throw new NotSupportedException(nameof(bitmap)) is not helpful. That exception should explain that only 32 bit ARGB images are supported.

IMHO, there's a bug retrieving the data in the Load() method. var size = bitData.Height * bitData.Stride; does not acknowledge that there are 32 bits per pixel. This should be var size = bitData.Height * bitData.Stride * (BitsPerPixel/8);. Write unit tests to confirm that your implementation works well.

Also note that the byte[] is not identical to the stored one. Your bitmap will be a square. Thus, when you originally store 17 bytes, you get back 25 bytes. And those extra bytes may contain garbage.

Is there any cases where padding would be an issue?

Padding in a Bitmap is called Stride. As Microsoft says:

The stride is the width of a single row of pixels (a scan line), rounded up to a four-byte boundary.

So, in your case, as long as you stick with 32 bit ARGB, stride and width are identical. But you have the square padding issue. A solution might be to store the array size in the first 32 bits of the image, adding that as an overhead.

Source Link
Thomas Weller
  • 1.7k
  • 14
  • 25
Loading