Proposal: change of handling 'char *' type arguments, especially for wxImage etc.

[toshinagata]

Sorry, this is a very long post...

[[ Background ]]
I am trying to write a program which programatically creates a wxImage and display it by using wxPaintDC:DrawBitmap(). However, I am running into several problems:
(1) I cannot find a good way to allocate a block of memory, modify its content, and pass it to a constructor of wxImage. Maybe wxMemoryBuffer is the solution.
(2) There are several constructors of wxImage that accept RGB data on memory, but the handling of the data are different among the constructors. wxImage(int width, int height, unsigned char* data, bool static_data = false) retrieves 'data' by lua_string() (which means only a Lua string can be given), whereas other versions retrieve 'data' by wxlua_getstringtype() (which accepts a Lua string and a wxString, but not other types).
(3) Then I tried to implement wxMemoryBuffer, and modify wxlua_getstringtype() to accept wxMemoryBuffer as 'const char*' data block. However, the return value of wxlua_getstringtype() (which has a type of 'const char*') is casted into wxCharBuffer before use. This causes data loss after the first NULL byte, because wxCharBuffer internally copies the given data by wxStrdup(). To fix this, I need to modify genwxbind.lua.

After struggling for a while, I made into the following set of modifications. I am not at all sure whether this is a good (or even acceptable) solution, so I would like it to be reviewed carefully. Thank you for your time.

[[ Issue 1: Handling of "char *" type arguments are changed ]]
[genwxbind.lua]
Currently, arguments with "char *" type (char *, const char *, unsigned char *, const unsigned char *) are processed with wxlua_getstringtype(), and the return value (whose type is "const char *") is casted into wxCharBuffer, and then casted back to the requested type. This causes a problem when the given argument has a NULL character.
Proposed fix: stop casting to wxCharBuffer, and use the return value of wxlua_getstringtype() directly and directly cast it into the required pointer type.

[[ Issue 2: Add definition of wxMemoryBuffer ]]
[wxbase_data.i]
Add definition of wxMemoryBuffer. Three new methods are implemented:
Byte(index, length = 1) -> returns the byte value (unsigned char) at the given index (0-based). If length larger than 1 is given, then multiple values are returned.
SetByte(index, data[, data2, data3, ...]) -> Set the byte data at the given index (0-based). If more than one data are given, they are set at the subsequent slots. Data length and buffer size are updated if necessary.
Fill(data, start_index, length) -> Set the same byte in the given range of the buffer. Data length and buffer size are updated if necessary.
[wxbase_override.hpp]
Implementation of wxLua_wxMemoryBuffer_Byte(), wxLua_wxMemoryBuffer_SetByte(), and wxLua_wxMemoryBuffer_Fill() are written.
[wxluasetup.h]
#define wxLUA_USE_wxMemoryBuffer 1 is added.

[[ Issue 3: Make wxMemoryBuffer usable as a 'string-type' argument ]]
[wxllua.cpp]
wxlua_getstringtype() and wxluaT_isuserdatatype() are modified, so that wxMemoryBuffer is accepted as a string-type argument.
wxlua_getstringtypelen() is implemented, so that the length of the 'string-type' argument can be retrieved whenever necessary.
[wxllua.h]
Declaration of wxlua_getstringtypelen() is added.

[[ Issue 4: Some wxImage methods are modified to allow wxString and wxMemoryBuffer as arguments for unsigned char* ]]
[wxcore_override.hpp]
wxLua_wxImageFromData_constructor(), wxLua_wxImage_SetAlphaData(), and wxLua_wxImage_SetData() are modified, so that they use wxlua_getstringtype() or wxlua_getstringtypelen() instead of lua_string() or lua_lstring().

[pkulchenko]

@toshinagata, I'll review the changes, but I don't see any issue with this approach so far.

However, I wonder if there is a simpler way to deal with this. I've ran into a similar issue with the plugin I was using for ZeroBrane Studio (https://github.com/pkulchenko/ZeroBranePackage/blob/master/screenshot.lua) and ended up using wx.wxMemoryInputStream and wx.wxPaintDC(panel):DrawBitmap() calls:

local function fileLoad(file)
  local f = FileRead(file)
  if not f then return end
  local fstream = wx.wxMemoryInputStream.new(f, #f)
  local log = wx.wxLogNull()
  local image = wx.wxImage()
  local loaded = image:LoadFile(fstream)
  return loaded and image or nil
end

--- and later....

  local panel = wx.wxPanel(ide:GetMainFrame(), wx.wxID_ANY,
    wx.wxDefaultPosition, wx.wxDefaultSize, wx.wxFULL_REPAINT_ON_RESIZE)
  panel:Connect(wx.wxEVT_PAINT, function()
      local dc = wx.wxPaintDC(panel)
      dc:DrawBitmap(wx.wxBitmap(image), 0, 0, true)
      dc:delete()
    end)

wx.wxMemoryInputStream.new() accepts a regular Lua string, so it can be modified in any way a Lua string can be modified (you also need to pass a string length though).

Would this be sufficient alternative or would the suggested wxMemoryBuffer mechanism be still required?

[toshinagata]

@pkulchenko, thank you for your comment. What I need is a 'mutable' block of bytes. More specifically, I want to do something like this:

function OnInit()
  buffer = wx.wxMemoryBuffer(320 * 200 * 3) -- 320x200 pixels, RGB buffer
  frame = wx.wxFrame(...)  --  Create the main frame
  frame:Connect(wx.wxEVT_PAINT, function (event)
    local dc = wx.wxPaintDC(frame)
    if bitmap then dc:DrawBitmap(bitmap, 0, 0, true) end
    dc:delete()
  end)
end
function OnTimer() -- Periodically called
  -- Content of buffer is modified every time OnTimer is called
  for x = 0, 319 do
    for y = 0, 199 do
      -- SomeFunc returns r, g, b data for the pixel
      buffer:SetByte((y * 320 + x) * 3, SomeFunc(x, y))
    end
  end
  image = wx.wxImage(320, 200, buffer, true)
  bitmap = wx.wxBitmap(image)
  frame:Refresh()
end

Modifying contents of the buffer is not possible in Lua strings or wxMemoryInputStream. So I think wxMemoryBuffer is useful in this respect.

[pkulchenko]

Proposed fix: stop casting to wxCharBuffer, and use the return value of wxlua_getstringtype() directly and directly cast it into the required pointer type.

@toshinagata, what is the difference between lua_tostring(L, 3) and wxlua_getstringtype(L, 3) if they return the same result with the same type (and the latter is using lua_tostring internally)? I think this is an acceptable solution, but I'd like to better understand the downside (if there is any). Maybe it's sufficient to replace wxCharBuffer with const char *, like you did in other places? Why do we need to change the API call?

(upon further review, it looks like the answer is that wxlua_getstringtype also handles wxString and wxMemoryBuffer types, so these can be used in the API calls in place of regular Lua strings, correct?)

I like the idea of added (wxlua-only methods), but have to think if this is what we need to allow, as wxlua is (mostly) a wrapper around wxwidgets. Are there any alternatives to Fill and Byte methods using the existing API? Also, should Byte be GetByte or did you name it to be similar to the string method?

I'll run some tests on this, but would be interested to know how you were able to test it? Do you want to extend one of the samples/ projects to add a demonstration on how it can be used?

Just to comment on the patch content, I think you did a great job of packaging it all together and figuring out wxlua details and the project structure!

[pkulchenko]

@toshinagata, I get two compilation warnings on Windows with gcc:

.../wxlua/wxLua/modules/wxbind/src/wxbase_data.cpp: In function
 'int wxLua_wxMemoryBuffer_Byte(lua_State*)':
.../wxlua/wxLua/modules/wxbind/src/wxbase_data.cpp:3660:43: war
ning: comparison between signed and unsigned integer expressions [-Wsign-compare]
     if (index + length > self->GetDataLen())
                                           ^
.../wxlua/wxLua/modules/wxbind/src/wxbase_data.cpp: In function
 'int wxLua_wxMemoryBuffer_SetByte(lua_State*)':
.../wxlua/wxLua/modules/wxbind/src/wxbase_data.cpp:3868:38: war
ning: comparison between signed and unsigned integer expressions [-Wsign-compare]
     if (self->GetDataLen() < index + length)
                                      ^

So far I don't see any other issues (I ran some application tests on the patched wxlua on Windows). there are couple of methods in wxMemoryBuffer that need to be marked with 2.9.4, but I'll do it after the merge.

[toshinagata]

@pkulchenko, thank you for your consideration.

(upon further review, it looks like the answer is that wxlua_getstringtype also handles wxString and wxMemoryBuffer types, so these can be used in the API calls in place of regular Lua strings, correct?)

Yes, that is exactly the point.

I like the idea of added (wxlua-only methods), but have to think if this is what we need to allow, as wxlua is (mostly) a wrapper around wxwidgets. Are there any alternatives to Fill and Byte methods using the existing API? Also, should Byte be GetByte or did you name it to be similar to the string method?

wxMemoryBuffer does not have methods for random access of the content. Probably we do not need them in C++, because we can easily access the content through the data pointer returned by wxMemoryBuffer::GetData(). This is not the case in Lua, so we do need new methods for accessing the content of wxMemoryBuffer. I chose 'Byte' instead of 'GetByte' to avoid confusion with GetData (which returns pointer, not the content), but now I think 'GetByte' should be more consistent with other method names in wxWidgets in general.

I'll run some tests on this, but would be interested to know how you were able to test it? Do you want to extend one of the samples/ projects to add a demonstration on how it can be used?

I'm not quite sure which sample is good to extend for that purpose...I'll try to post a short sample program that uses wxMemoryBuffer and wxImage as I suggested before.

Just to comment on the patch content, I think you did a great job of packaging it all together and figuring out wxlua details and the project structure!

I remember I posted a horrible patch when I first tried to contribute to your project; I have learned a little bit since then :-)

[pkulchenko]

@toshinagata, I made several changes and pushed them all into a new branch:

1. Renamed Byte to GetByte
2. Marked two methods with 2.9.4
3. Added wxSTC methods that require wxMemoryBuffer
4. Removed comparison warnings (by rewriting some of the comparisons, so check the logic)

Everything looks good so far and added wxSTC methods work correctly for me.

Take a look at the results and if everything looks good, I'll merge the changes.

[toshinagata]

@pkulchenko, thank you for your efforts. Your patch looks all right so far.

I am posting below a sample program that uses wxImage and wxMemoryBuffer. If you find it useful, please consider including it in the samples directory. (It may not work... I usually use wxLua as an embedded environment in my own application, so I am not quite sure whether it works on plain wxLua.)

mandelbrot.wx.lua.txt

[pkulchenko]

@toshinagata, thank you! The example works for me and looks good, so I committed it into the samples/ folder. I made couple of tweaks to eliminate the flicker, take a look.

[toshinagata]

@pkulchenko, thank you!