Skip to content

Conversation

@loicpoulain
Copy link
Contributor

@loicpoulain loicpoulain commented Jul 1, 2019

DEPENDS on HAL_NXP change: zephyrproject-rtos/hal_nxp#6

PRESENTATION: https://docs.google.com/presentation/d/1j44YHUqynN-Vw67NgHHx741yzLIbjbc76tcObi8J3Zc/edit?usp=sharing

  • Generic Video API
  • MCUX CSI Video driver (CMOS Sensor Interface) => Video Capture driver
  • MT9M114 Video camera driver => Video control driver
  • SW Video pattern generator => Video Capture driver
  • A basic capture sample application, counting received frames
  • A tpcserversink sample pushing video to a TCP client

@loicpoulain loicpoulain added area: API Changes to public APIs area: Video Video subsystem platform: NXP NXP area: camera area: Samples Samples labels Jul 1, 2019
@loicpoulain loicpoulain changed the title Video Video API and driver Jul 1, 2019
@zephyrbot zephyrbot added area: Devicetree area: Boards EXT Has change or related to ext/ (obsolete) labels Jul 1, 2019
@zephyrbot
Copy link

zephyrbot commented Jul 1, 2019

All checks are passing now.

checkpatch (informational only, not a failure)

-:276: WARNING:LEADING_SPACE: please, no spaces at the start of a line
#276: FILE: drivers/video/Kconfig.mt9m114:10:
+        bool "MT9M114 Aptina CMOS digital image sensor"$

-:277: WARNING:LEADING_SPACE: please, no spaces at the start of a line
#277: FILE: drivers/video/Kconfig.mt9m114:11:
+        depends on I2C$

-:278: WARNING:LEADING_SPACE: please, no spaces at the start of a line
#278: FILE: drivers/video/Kconfig.mt9m114:12:
+        help$

-:279: WARNING:LEADING_SPACE: please, no spaces at the start of a line
#279: FILE: drivers/video/Kconfig.mt9m114:13:
+          Enable driver for MT9M114 CMOS digital image sensor device.$

-:295: WARNING:LEADING_SPACE: please, no spaces at the start of a line
#295: FILE: drivers/video/Kconfig.sw_generator:10:
+        bool "Video Software Generator"$

-:296: WARNING:LEADING_SPACE: please, no spaces at the start of a line
#296: FILE: drivers/video/Kconfig.sw_generator:11:
+        help$

-:297: WARNING:LEADING_SPACE: please, no spaces at the start of a line
#297: FILE: drivers/video/Kconfig.sw_generator:12:
+          Enable video pattern generator (for testing purpose).$

-:890: WARNING:LONG_LINE: line over 80 characters
#890: FILE: drivers/video/video_mcux_csi.c:116:
+	data->csi_config.polarityFlags = kCSI_HsyncActiveHigh | kCSI_DataLatchOnRisingEdge;

-:891: WARNING:LONG_LINE_COMMENT: line over 80 characters
#891: FILE: drivers/video/video_mcux_csi.c:117:
+	data->csi_config.workMode = kCSI_GatedClockMode; /* use VSYNC, HSYNC, and PIXCLK */

-:989: WARNING:TYPO_SPELLING: 'ouput' may be misspelled - perhaps 'output'?
#989: FILE: drivers/video/video_mcux_csi.c:215:
+		/* Flush driver ouput queue */

-:1275: WARNING:LINE_SPACING: Missing a blank line after declarations
#1275: FILE: drivers/video/video_sw_generator.c:84:
+			int color_idx =  data->ctrl_vflip ? 7 - w / bw : w / bw;
+			if (data->fmt.pixelformat == VIDEO_PIX_FMT_RGB565) {

-:1575: WARNING:LONG_LINE_COMMENT: line over 80 characters
#1575: FILE: include/drivers/video-controls.h:33:
+#define VIDEO_CTRL_CLASS_GENERIC	0x00000000	/* Generic class controls */

-:1576: WARNING:LONG_LINE_COMMENT: line over 80 characters
#1576: FILE: include/drivers/video-controls.h:34:
+#define VIDEO_CTRL_CLASS_CAMERA		0x00010000	/* Camera class controls */

-:1577: WARNING:LONG_LINE_COMMENT: line over 80 characters
#1577: FILE: include/drivers/video-controls.h:35:
+#define VIDEO_CTRL_CLASS_MPEG		0x00020000	/* MPEG-compression controls */

-:1578: WARNING:LONG_LINE_COMMENT: line over 80 characters
#1578: FILE: include/drivers/video-controls.h:36:
+#define VIDEO_CTRL_CLASS_JPEG		0x00030000	/* JPEG-compression controls */

-:1579: WARNING:LONG_LINE_COMMENT: line over 80 characters
#1579: FILE: include/drivers/video-controls.h:37:
+#define VIDEO_CTRL_CLASS_VENDOR		0xFFFF0000	/* Vendor-specific class controls */

-:1582: WARNING:LONG_LINE_COMMENT: line over 80 characters
#1582: FILE: include/drivers/video-controls.h:40:
+#define VIDEO_CID_HFLIP			(VIDEO_CTRL_CLASS_GENERIC + 0) /* Mirror the picture horizontally */

-:1583: WARNING:LONG_LINE_COMMENT: line over 80 characters
#1583: FILE: include/drivers/video-controls.h:41:
+#define VIDEO_CID_VFLIP			(VIDEO_CTRL_CLASS_GENERIC + 1) /* Mirror the picture vertically */

-:1666: WARNING:TYPO_SPELLING: 'suported' may be misspelled - perhaps 'supported'?
#1666: FILE: include/drivers/video.h:59:
+ * @param pixelformat is a list of suported pixel formats (0 terminated).

-:1903: WARNING:LONG_LINE_COMMENT: line over 80 characters
#1903: FILE: include/drivers/video.h:296:
+ * Enqueue an empty (capturing) or filled (output) video buffer in the driver’s

-:1987: WARNING:LONG_LINE_COMMENT: line over 80 characters
#1987: FILE: include/drivers/video.h:380:
+ * video_stream_start is called to enter ‘streaming’ state (capture, output...).

-:2154: WARNING:LONG_LINE: line over 80 characters
#2154: FILE: include/drivers/video.h:547:
+	((u32_t)(a) | ((u32_t)(b) << 8) | ((u32_t)(c) << 16) | ((u32_t)(d) << 24))

-:2157: WARNING:LONG_LINE_COMMENT: line over 80 characters
#2157: FILE: include/drivers/video.h:550:
+#define VIDEO_PIX_FMT_BGGR8  video_fourcc('B', 'G', 'G', 'R') /*  8  BGBG.. GRGR.. */

-:2158: WARNING:LONG_LINE_COMMENT: line over 80 characters
#2158: FILE: include/drivers/video.h:551:
+#define VIDEO_PIX_FMT_GBRG8  video_fourcc('G', 'B', 'R', 'G') /*  8  GBGB.. RGRG.. */

-:2159: WARNING:LONG_LINE_COMMENT: line over 80 characters
#2159: FILE: include/drivers/video.h:552:
+#define VIDEO_PIX_FMT_GRBG8  video_fourcc('G', 'R', 'B', 'G') /*  8  GRGR.. BGBG.. */

-:2160: WARNING:LONG_LINE_COMMENT: line over 80 characters
#2160: FILE: include/drivers/video.h:553:
+#define VIDEO_PIX_FMT_RGGB8  video_fourcc('R', 'G', 'G', 'B') /*  8  RGRG.. GBGB.. */

-:2163: WARNING:LONG_LINE_COMMENT: line over 80 characters
#2163: FILE: include/drivers/video.h:556:
+#define VIDEO_PIX_FMT_RGB565 video_fourcc('R', 'G', 'B', 'P') /* 16  RGB-5-6-5 */

-:2851: WARNING:LONG_LINE: line over 80 characters
#2851: FILE: soc/arm/nxp_imx/rt/dts_fixup.h:236:
+#define DT_VIDEO_MCUX_CSI_BASE_ADDRESS		DT_NXP_IMX_CSI_402BC000_BASE_ADDRESS

-:2853: WARNING:LONG_LINE: line over 80 characters
#2853: FILE: soc/arm/nxp_imx/rt/dts_fixup.h:238:
+#define DT_VIDEO_MCUX_CSI_IRQ_PRI		DT_NXP_IMX_CSI_402BC000_IRQ_0_PRIORITY

-:2855: WARNING:LONG_LINE: line over 80 characters
#2855: FILE: soc/arm/nxp_imx/rt/dts_fixup.h:240:
+#define DT_VIDEO_MCUX_CSI_SENSOR_NAME		DT_NXP_IMX_CSI_402BC000_SENSOR_LABEL

- total: 0 errors, 30 warnings, 2636 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: AVOID_EXTERNS BRACES CONFIG_EXPERIMENTAL CONST_STRUCT DATE_TIME FILE_PATH_CHANGES MINMAX NETWORKING_BLOCK_COMMENT_STYLE PRINTK_WITHOUT_KERN_LEVEL SPLIT_STRING VOLATILE

NOTE: If any of the errors are false positives, please report
      them to the maintainers.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

@loicpoulain loicpoulain force-pushed the video branch 2 times, most recently from db3ab12 to 3720757 Compare July 1, 2019 15:34
@loicpoulain loicpoulain changed the title Video API and driver Video API, Driver and Sample Jul 1, 2019
@loicpoulain loicpoulain force-pushed the video branch 3 times, most recently from 10f0fdd to b991ac7 Compare July 5, 2019 16:22
@loicpoulain
Copy link
Contributor Author

Hi all, any comments on this PR?

@loicpoulain loicpoulain changed the title Video API, Driver and Sample Video API, Drivers and Sample Jul 5, 2019
@microbuilder
Copy link
Member

Hi all, any comments on this PR?

I think with the 4th of July weekend and summer holidays a lot of people are out. I'll try to have a look at this Monday myself, though.

@gon1332
Copy link
Contributor

gon1332 commented Jul 8, 2019

That's the first time I see this API, so I don't think I can comment more, until I actually try to port my camera module to it.
So, it looks good to me for now.

Copy link
Contributor

@pizi-nordic pizi-nordic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me there are few blank areas in this API definition. I will try to highlight them using he following example:

[ Camera ] => [ Software scaler/filter/color space converter ] => [ HW Encoder ] => Storage

Assumptions:

  • All components in the brackets could (and should) be represented using this API.

Questions:

  • Each of the device provides its own video frame allocator. Am I supposed to reallocate frame and copy its contents manually between each processing step or I can pass the frame directly?
  • I assume that I enqueue uncompressed frame to the HW encoder and dequeue the compressed one. Since most of the video compression algorithms do not support compression in place, the HW encoder will allocate the output frame. Who is responsible for freeing the input frame? If user of the API, how he/she could know when the frame is no longer used. If the HW encoder driver, how it could know which driver is owner of the frame and which version of video_api_release_frame_t should be called?
  • Some of the video encoders need some kind of out of band data describing properties of the compressed stream (the H264 is good example). How we are supposed to retrieve this data from the encoder and pass it to the decoder? Will we have concept of the stream in the API?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this API valid for YUV data with color subsampling?

@loicpoulain loicpoulain requested a review from dbkinder July 10, 2019 18:32
@carlescufi
Copy link
Member

From the API meeting:

  • @jfischer-phytec-iot can take a look post-ELCE (November timeframe)
  • @MaureenHelm will take another look
  • An option is to merge this by marking it as experimental in Kconfig and Doxygen
  • Deadline of 22nd of October to merge as experimental if there is no further activity

Copy link
Member

@MaureenHelm MaureenHelm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tried sending video to the LCD display?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be updated

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks done

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stray change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, fixed now

Loic Poulain added 11 commits October 17, 2019 08:12
This generic video API can be used to capture/output video frames.

Once a video buffer is enqueued to a video device endpoint, device owns
the buffer and can process to capture (camera), output (disk, display),
convert (hw encoder)... User can then call dequeue to retrieve
the processed buffer (video driver ensure cache coherency).

Once dequeued, video buffer is owned by user (e.g. for frame
processing, display buffer update, write to media, etc...).

For each video-buffer, user needs allocate the associated frame buffer
via video_buffer_alloc. Buffer format is defined by video device
endpoint configuration. Video device can be controlled (e.g. contrast,
brightness, flip...) via controls.

Signed-off-by: Loic Poulain <[email protected]>
Add support for CMOS Sensor Interface video driver.

Signed-off-by: Loic Poulain <[email protected]>
Add CSI node to generic nxp rt dtsi.
Add corresponding dts binding.
Add CSI capability for rt MCUs.

Signed-off-by: Loic Poulain <[email protected]>
This enables CSI node, and configures pinmux when driver is enabled.

Signed-off-by: Loic Poulain <[email protected]>
Add myself to owner of drivers/video.

Signed-off-by: Loic Poulain <[email protected]>
MT9M114 is a CMOS digital image sensor.
Implement video interface.
Only VGA (640x480) supported for now.

Signed-off-by: Loic Poulain <[email protected]>
Sensor can be connected to the 24pin camera connector.

Signed-off-by: Loic Poulain <[email protected]>
This is a virtual device generating video pattern for testing
purpose. It supports colobar pattern for now.

Signed-off-by: Loic Poulain <[email protected]>
Simple video sample getting frames from video capture device.
Tested with mimxrt1064_evk and MT9M114 sensor.

Signed-off-by: Loic Poulain <[email protected]>
This samples captures frames from video capture device (in any format),
and sends them to its TCP client.

Tested with:
 - mimxrt1064 + MT9M114 video sensor.
 - Gstreamer 1.8.3 running on host

Signed-off-by: Loic Poulain <[email protected]>
Keep flat video driver directory for now.

Signed-off-by: Loic Poulain <[email protected]>
@loicpoulain
Copy link
Contributor Author

Have you tried sending video to the LCD display?

No, I do not have LCD for the iMX RT, but AFAIR, @JunYangNXP knows quite well how to do this, not sure if he has time, but it would be a good end-to-end sample to add.

@wentongwu wentongwu self-requested a review October 17, 2019 06:24
@MaureenHelm
Copy link
Member

Have you tried sending video to the LCD display?

No, I do not have LCD for the iMX RT, but AFAIR, @JunYangNXP knows quite well how to do this, not sure if he has time, but it would be a good end-to-end sample to add.

I'm wondering about the interaction between this video api and the existing display api in include/drivers/display.h. Will they work together? Would it make sense to build a video output driver on top of the display api?

There is a display driver for iMX RT in drivers/display/display_mcux_elcdif.c

@loicpoulain
Copy link
Contributor Author

loicpoulain commented Oct 18, 2019

Have you tried sending video to the LCD display?

No, I do not have LCD for the iMX RT, but AFAIR, @JunYangNXP knows quite well how to do this, not sure if he has time, but it would be a good end-to-end sample to add.

I'm wondering about the interaction between this video api and the existing display api in include/drivers/display.h. Will they work together? Would it make sense to build a video output driver on top of the display api?

It's certainly do-able yes, the APIs are quite similar and a glue driver is possible. I'll also investigate if it would be worth to convert display API to video one. But it's a long term task. There is steps in between like making both API including same pixformat, fourcc definitions, etc...

There is a display driver for iMX RT in drivers/display/display_mcux_elcdif.c

Ok, I'm going to try getting one.

@carlescufi
Copy link
Member

Have you tried sending video to the LCD display?

No, I do not have LCD for the iMX RT, but AFAIR, @JunYangNXP knows quite well how to do this, not sure if he has time, but it would be a good end-to-end sample to add.

I'm wondering about the interaction between this video api and the existing display api in include/drivers/display.h. Will they work together? Would it make sense to build a video output driver on top of the display api?

There is a display driver for iMX RT in drivers/display/display_mcux_elcdif.c

@vanwinkeljan could you please take a look at the interaction between this new API and the existing display API?

@vanwinkeljan
Copy link
Member

@vanwinkeljan could you please take a look at the interaction between this new API and the existing display API?

@carlescufi / @MaureenHelm It looks pretty straightforward to build a glue logic video endpoint on top of the existing display API. But I think there is still a use case to support both APIs; to me it looks like the video API is intended to operate on full screen size and update the complete display at once whereas the display API allows to do partial updates, this feature could be of importance to memory constrained devices. Of Course the video API could be extended to support this but I would not block this PR for that.

@loicpoulain
Copy link
Contributor Author

.

@vanwinkeljan could you please take a look at the interaction between this new API and the existing display API?

@carlescufi / @MaureenHelm It looks pretty straightforward to build a glue logic video endpoint on top of the existing display API. But I think there is still a use case to support both APIs; to me it looks like the video API is intended to operate on full screen size and update the complete display at once whereas the display API allows to do partial updates, this feature could be of importance to memory constrained devices. Of Course the video API could be extended to support this but I would not block this PR for that.

Thanks for your review, indeed it's easy to feed the display API with video API buffer content. Let's see later if there is a need and a way to extend video API to include display/graphics capabilities. For now we keep both and I'll submit a sample using Video API for capture + colorspace conversion and Display API for rendering.

@carlescufi
Copy link
Member

@vanwinkeljan could you please take a look at the interaction between this new API and the existing display API?

@carlescufi / @MaureenHelm It looks pretty straightforward to build a glue logic video endpoint on top of the existing display API. But I think there is still a use case to support both APIs; to me it looks like the video API is intended to operate on full screen size and update the complete display at once whereas the display API allows to do partial updates, this feature could be of importance to memory constrained devices. Of Course the video API could be extended to support this but I would not block this PR for that.

Thank you @vanwinkeljan for the review.
@MaureenHelm would you mind approving the PR or removing your NACK?

@carlescufi carlescufi dismissed pizi-nordic’s stale review October 23, 2019 12:04

stale, not involved in the project anymore

@MaureenHelm MaureenHelm merged commit 14a0def into zephyrproject-rtos:master Oct 25, 2019
@nashif nashif added the Release Notes To be mentioned in the release notes label Nov 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: API Changes to public APIs area: Boards area: camera area: Devicetree area: Documentation area: Modules area: Samples Samples area: Video Video subsystem EXT Has change or related to ext/ (obsolete) platform: NXP NXP Release Notes To be mentioned in the release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.