Skip to content

raw_mjpeg pixel_format corrupts image data — process_image() memcpy uses fixed size_in_bytes instead of the real bytes_used #389

Description

@gearsincorg

Bug summary

pixel_format: "raw_mjpeg" produces corrupted image data on /image_raw and (via the standard image_transport compressed republisher) on /image_raw/compressed too. The corruption looks like a narrow band of visual noise at the top of the frame followed by a solid color fill for the rest of the image, and is 100% reproducible on every frame.

Confirmed present in the 0.8.1 tag (currently the latest release, packaged as ros-jazzy-usb-cam) and still present on main as of today.

Root cause

UsbCam::process_image() in src/usb_cam.cpp:

void UsbCam::process_image(const char * src, char * & dest, const int & bytes_used)
{
  // TODO(flynneva): could we skip the copy here somehow?
  // If no conversion required, just copy the image from V4L2 buffer
  if (m_image.pixel_format->requires_conversion() == false) {
    memcpy(dest, src, m_image.size_in_bytes);
  } else {
    m_image.pixel_format->convert(src, dest, bytes_used);
  }
}

For a pixel format with requires_conversion() == false, this copies a fixed number of bytes (m_image.size_in_bytes, computed from width * height * channels * byte_depth) instead of the actual bytes_used argument that's passed in (the real per-frame length V4L2's VIDIOC_DQBUF reports via buf.bytesused).

For a genuinely fixed-size raw format (real YUYV, RGB8, etc.) bytes_used always equals size_in_bytes, so this is harmless. But raw_mjpeg (RAW_MJPEG in include/usb_cam/formats/mjpeg.hpp) is also marked requires_conversion() == false, and MJPEG is a compressed, variable-length payload — its real bytes_used is typically a small fraction of the fixed size computed for that resolution (e.g. on my camera at 800x600, real JPEG frames were ~45-100KB vs. a computed size_in_bytes of ~960KB).

The result: this memcpy reads ~10-20x past the end of the valid captured data, into whatever stale/uninitialized memory happens to follow it in the mmap'd V4L2 buffer. That garbage then gets published as /image_raw (mislabeled with an encoding like yuv422 even though it's actually raw JPEG bytes followed by garbage), and /image_raw/compressed — produced by image_transport's standard compressed-image republisher doing a real cv::imencode of that same mislabeled buffer — ends up as a syntactically valid JPEG that faithfully encodes the wrong data: the real JPEG bytes (a small fraction of the oversized buffer) show up as noise at the start of the image, and the much larger uninitialized remainder encodes as a solid color.

Steps to reproduce

  1. Any UVC camera supporting MJPG.
  2. usb_cam_node_exe with pixel_format: "raw_mjpeg", any resolution.
  3. View /image_raw or /image_raw/compressed.

Verified independent of:

  • Resolution — reproduced identically at both 640x480 and 800x600.
  • Frame rate — still reproduced with framerate throttled to 2 fps, ruling out any camera-encoder-under-load theory.
  • V4L2 negotiationv4l2-ctl --get-fmt-video reports an identical Size Image regardless of which pixel_format is selected; a full debug-level startup log diff between a working (mjpeg2rgb) and broken (raw_mjpeg) run is identical apart from the one line naming the pixel format. So this isn't a device negotiation difference — it's purely the post-capture memcpy.

Expected behavior

raw_mjpeg should publish the camera's actual native MJPEG bytes unmodified (that appears to be its intent, per formats/mjpeg.hpp's RAW_MJPEG class), sized to the real per-frame bytes_used, not a fixed size.

Suggested fix

In the requires_conversion() == false branch, copy bytes_used bytes instead of m_image.size_in_bytes:

memcpy(dest, src, bytes_used);

This should be safe for genuine fixed-size raw formats too, since bytes_used == size_in_bytes for those.

Related: dead code in UsbCamNode::update()

Separately, and possibly why this hasn't been caught: src/usb_cam_node.cpp dispatches on the literal string "mjpeg" in a few places (e.g. update()):

bool isSuccessful = (m_parameters.pixel_format_name == "mjpeg") ?
  take_and_send_image_mjpeg() :
  take_and_send_image();

But no pixel format is ever actually registered/named "mjpeg" — the real options for an MJPEG-capturing camera are "raw_mjpeg" and "mjpeg2rgb" (include/usb_cam/formats/mjpeg.hpp). So take_and_send_image_mjpeg() — which looks like it might have been intended as a true zero-copy MJPEG passthrough — is unreachable dead code for any valid configuration; take_and_send_image() is always used instead, which is what actually triggers the bug above. (For what it's worth, take_and_send_image_mjpeg() calls the same buggy process_image() internally, so fixing the memcpy above fixes both paths regardless of whether this dispatch string is also corrected.)

Environment

  • ros-jazzy-usb-cam 0.8.1 (Ubuntu Noble / ROS 2 Jazzy), also reproduced by inspection against current main
  • Raspberry Pi 4B, aarch64
  • Generic UVC webcam supporting MJPG up to 1920x1080@30

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions