#7 rgb24 support

[Jarek Czekalski]

Similarly to vfw, I had to also adapt dshow to talk to my camera connected to W2K. It provides MEDIASUBTYPE_RGB24.

Our flipping tactics work well in this case also. I attach a patch if you would like to change something before I commit it.

I also need to test this version with mjpg camera first, so I published a binary in https://sourceforge.net/projects/zbarw/files/test/

[klaus triendl]

Could we make this more generic? If your camera's source is
MEDIASUBTYPE_RGB24, I suppose that other uncompressed formats like
rgb32, rgb555 and rgb565 are possible.

What if we extend the mjpg mapping machinery?

[Jarek Czekalski]

Looks like I have little time these days, so I can only answer now (thinking about your release), work later. I don't know if you want to have this functionality in your release. If this is the case I could commit it as is, and you make further changes. I'll be available again about 15:00.

Now to the subject:

more generic?

Sure, I think the design allows for easy extension, provided that no data conversion is necessary. But in the first step there is only single format support.

What if we extend the mjpg mapping machinery?

The mapping machinery handles conversion between different formats. Here we have one format with different symbols. So that's the difference that I had in mind when deciding not to use the mapping.
Do you think there is a better way of handling rgb24? Please give more details then, maybe a sketch of the solution.

[Jarek Czekalski]

[Jarek Czekalski]

[Jarek Czekalski]

Klaus, I analyzed the differences between my rgb24_1 and your suggestion. Here are the differences enumerated and commented.

1. print_bih_info and fmt_str
   The added value of this large part of code is not very significant. Definitely it needs a separate patch. And having a new function gives a side effect:
   print_bih_info: setting camera format: YUY2(32595559) 64
   Instead of "print_bih_info" the caller's name should be here.
   If we improve the supported/unsupported detection, this part may be even unnecessary, because unsupported formats are handled verbosely enough.

2. dshow_cacces_bih: no comments

3. removal of fourcc member.
   I don't think it's a good change. Compression is a different value and fourcc is something different, in case of BI_RGB for example. And without fourcc member the check for null structure is much less readable. I would leave fourcc member just for the sake of code readability.

4. Changing the names of other int_format_s members: no comments

5. get_int_format_index, @param fmt0 description - good idea

6. get_int_format_index changed because of lack of fourcc: better without this change

7. get_zbar_format_for_bih - This was actually meant to be only the translator between different coding of the same formats, not for the mapping. I changed the name of the function to get_fourcc_for_bih to avoid ambiguity.
   You inserted mapping betweend BGR3 and MJPG here. It is a different thing, so it should be handled in another function.

8. prepare_mjpg_format_mapping change
   This change is a consequence of get_zbar_format_for_bih change, so I temporarily "reject it". It should be reconsidered. Note also that the check:
   if (state->int_formats[i].biCompression != vdo->formats[i])
   is suspicious, because biCompression is in Windows coding and vdo->formats[i] is in zbar fourcc coding. Are these 2 comparable?
   The same possibility of a mistake is here:
   get_int_format_index(state->int_formats, vdo->formats[i]);

9. if ((bih->biCompression != int_fmt->biCompression ) ||
   (bih->biBitCount != int_fmt->biBitCount ) )
   At the moment I'm leaving previous solution.

10. if (is_supported)
    Thanks for catching the bug, however I added a comment:
    // This is actually a check if the format is recognized.
    // TODO: Check if the format is really supported by zbar.

I think I'll commit my rgb24 solution that was enreached by your suggestions. The suggestions not included iny my commit may be subject to further discussion. Is it ok, Klaus? I need more time to test it again after the changes, so this will happen no sooner than tomorrow evening.

[klaus triendl]

Hi Jarek,

I'm afraid my viewpoint didn't reach you, at least that's what comes out of your comments.
I really would like to hear some comments regarding the whole issue, not just whether each single change is indeed a good idea or not.

We have different viewpionts here, so I'd like to convey mine to you:

1. have a list of internal formats and a list of zbar formats, which constitutes the mappings.
   Now, the internal format is always the biCompression/biBitCount pair from the BITMAPINFOHEADER to be able to combine fourcc and uncompressed bitmaps. The biCompression may be the fourcc code for compressed video formats as documented at http://msdn.microsoft.com/en-us/library/windows/desktop/dd318229(v=vs.85).aspx. Hence, fourcc is included in this solution.
2. Do the mapping in one function, which happens to be named get_zbar_format_for_bih.
   Note that I don't consider RGB24==BGR3, therefore I think that we have a mapping here from RGB24->BGR3. Of course, this function naturally takes care of the MJPG->BGR4 mapping.

In this sense:
- The added value of the print_bih_info is very significant because it is part of the abstraction from fourcc only.
- Whether checking for null structure is less readable is not the big question, I think it's just another form of end condition.
- And I didn't intend to change prepare_mjpg_format_mapping but as my naming of the function drop_mapped_formats_infavor_of_native suggests it's actually a new function and prepare_mjpg_format_mapping isn't needed anymore. Unluckily the diff makes you believe something else.

[Jarek Czekalski]

Note that I don't consider RGB24==BGR3

Klaus, why do you say that RGB24!=BGR3 ? We definitely can't find agreement if you consider RGB24 a different format than BGR3. For me it's the same format with a different coding of its name.

For me 2 formats are the same if the image data is the same. If, and only if, the 2 formats are different a conversion routine is needed, which transforms image data. I'm very curious what is your definition of same formats.

[klaus triendl]

Hi Jarek,

well that's a good question. I didn't find anything about fourcc BGR3 or BGR4, it doesn't seem to be an official fourcc code. And since the microsoft documentation states that the BITMAPINFOHEADER's biCompression member is a fourcc code for compressed formats but is not for uncompressed bitmaps I concluded RGB24!=BGR3.
Another argument is that a mapping doesn't necessarily mean that we need a conversion routine, and we can treat the camera format as is, no matter whether fourcc or any other format system we encounter.
This is hypothetical of course, but if we imagine that there's one day a microsoft format for which we don't have a fourcc yet, it seems more logical to me to treat the format as is rather than finding a corresponding or a very newly invented fourcc for it. This is what microsoft concluded because they invented the subformat GUIDs.

Independent from whether RGB24==BGR3 we now have 2 ways:
1. determine a fourcc code from the biCompression/biBitCount (routine get_fourcc_for_bih) and store it as part of the internal format, handle MJPG explicitly
2. store only the biCompression/biBitCount as internal format and translate to the format provided to zbar via the get_zbar_format_for_bih routine, including MJPG, no matter whether we need a conversion routine.

I personally favor 2. because it seems to me more generic. On the other hand I guess that you have strong reasons for 1.

Either way our discussion is bearing fruit, leading to a good solution.

[Jarek Czekalski]

Klaus, having fourcc in int_format_s doesn't make solution less generic. It just cleanly describes a case when microsoft uses different coding than zbar (BGR3 example). And helps a lot in code simplifying.

Please try to do something with point 8 in my long comment above. It shows that your solution is not correct.

I hope that after analyzing point 8 you'll come to a conclusion, that for now we don't need to complicate things a lot and leave a simple solution with fourcc member of int_format_s. Further extending of code is possible, but I prefer to commit the simplest solution first.

BGR3 is not very official code, but describes well the pixel format. Better than Microsoft's non-fourcc way. Anyway what Microsoft marks as RGB24 is exactly what zbar marks with BGR3. Otherwise my camera wouldn't work.

[klaus triendl]

There's not much to say about 8., the code isn't a mistake. Please note again that I didn't intend to change prepare_mjpg_format_mapping but the goal was to introduce a new function with respect to my rgb24 implementation.

Explaining the workings of drop_mapped_formats_infavor_of_native:

• It iterates the list of formats
• tests whether the zbar format differs from the internal (=> mapping)
• if yes
  • searches the mapped format within the internal formats (native format)
  • if found
    • drops the mapping in favor of the native one
  • if not found
    • continue
• if no
  • continue

I agree with you that we shouldn't complicate things. As I know you you will well document the code such that your intentions are clear.

The only important wish I have - don't know whether you've already integrated this in your latest code: get_fourcc_for_bih should return 0 for all uncompressed bitmaps other than RGB24.

[Jarek Czekalski]

There's not much to say about 8., the code isn't a mistake.

I have an imporession that you did not analyzed my comment, but assumed I must be wrong. Anyway I have put it now in more detailed way.

Assume that we have camera providing both RGB32 (BGR4) and MJPG formats. Old code would remove MJPG format in favor of native BGR4. Your code will fail to do it because:

 int iConv = get_int_format_index(state->int_formats, vdo->formats[i]);

BGR4 can never be found by get_int_format_index because it tries to locate BGR4 entry in biCompression member. BGR4 is expressed by Windows as BI_RGB with 32 bit count, not by BGR4 fourcc.

The only important wish I have - don't know whether you've already integrated this in your latest code: get_fourcc_for_bih should return 0 for all uncompressed bitmaps other than RGB24.

There is little difference between returning 0 and non-zero in these cases. Compare it to camera providing fourcc format unknown to zbar like CPLA. So is this really important to return 0 for "uncompressed bitmaps"? And to be strict, Windows specifications considers only BI_RGB uncompressed format, which already has 0 value.

[klaus triendl]

BGR4 can never be found by get_int_format_index because it tries to locate BGR4 entry in biCompression member. BGR4 is expressed by Windows as BI_RGB with 32 bit count, not by BGR4 fourcc.

Well now, you've caught me :)

There is little difference between returning 0 and non-zero in these cases. Compare it to camera providing fourcc format unknown to zbar like CPLA. So is this really important to return 0 for "uncompressed bitmaps"? And to be strict, Windows specifications considers only BI_RGB uncompressed format, which already has 0 value.

Well, for me it's a question of explicit clarity. While programming with named constants we don't really know that BI_RGB==0 by just looking at the code. And 0 has a special meaning because it serves as a list terminator. Furthermore we may encounter uncompressed bitmaps with biCompression==BI_BITFIELDS which is neither 0 nor a fourcc code.
So my intention is that when the function returns zero, the format is unsupported and doesn't make it into the format list at all.

[Jarek Czekalski]

As I said before, the difference is minimal, but I think I'll do something that will be better and will be along with your suggestion. Instead of get_fourcc_for_bih will have get_fourcc_for_mt. Inside it your fourcc detection code will be used. Known types like MEDIATYPE_RGB24 will also be caught and interpreted.

I'll submit a patch here so thay you could have a look at it.

[Jarek Czekalski]

Committed as 405:42cfe811c463

From Klaus' proposal the following were not included:
1. print_bih_info and fmt_str
2. removal of fourcc member.
3. prepare_mjpg_format_mapping related changes

Klaus, commit print_bih_info if you wish.