- 
                Notifications
    You must be signed in to change notification settings 
- Fork 8.2k
stm32: addition of the STM32 JPEG Codec (NV12 -> JPEG encoding) #96678
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
63ec263    to
    76d0e06      
    Compare
  
    76d0e06    to
    5b55717      
    Compare
  
            
          
                drivers/video/video_stm32_jpeg.c
              
                Outdated
          
        
      | /* Check that input / output formats are correct */ | ||
| if ((data->m2m.in.fmt.pixelformat == VIDEO_PIX_FMT_JPEG && | ||
| data->m2m.out.fmt.pixelformat == VIDEO_PIX_FMT_JPEG) || | ||
| (data->m2m.in.fmt.pixelformat != VIDEO_PIX_FMT_JPEG && | ||
| data->m2m.out.fmt.pixelformat != VIDEO_PIX_FMT_JPEG)) { | ||
| LOG_ERR("One of input or output format must be JPEG"); | ||
| ret = -EINVAL; | ||
| goto out; | ||
| } | ||
|  | ||
| /* FIXME - temporary until the decoder support get added */ | ||
| if (data->m2m.in.fmt.pixelformat == VIDEO_PIX_FMT_JPEG) { | ||
| LOG_ERR("Decoder not yet implemented"); | ||
| ret = -EIO; | ||
| goto out; | ||
| } | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: These two checks operate on global data rather than common so maybe they can be done somewhere else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually we do those checks at the set_stream time right before starting the device. I could have also block (the decoder) at set_fmt time, but in such case I have need to distinguish input / output in the supported format table so I found it less intrusive to put this FIXME (and the check) here, considering that this JPEG check is going to disappear when the decoder will be implemented
0946ea7    to
    078a3a0      
    Compare
  
    078a3a0    to
    3097dac      
    Compare
  
    | return ret; | ||
| } | ||
|  | ||
| static int stm32_jpeg_set_stream(const struct device *dev, bool enable, enum video_buf_type type) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems set_stream() does nothing (related to the HW). Should we merge stm32_jpeg_start_codec() into this ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, actually start_codec, (maybe badly named) is processing frames, so it is called from enqueue (as well), however, indeed, within the set_stream, we might also call start_codec is there are already buffers queued into both input & output. Adding this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But we could call set_stream() inside enqueue() as well ?
Like the PxP, I think this JPEG HW is kind of one-shot device so there is no clear concept for "start streaming" as we need to (re)start the process for each frame / image, so the real start will be launched whenever there are enough buffers. But this is a small point, up to you to choose merge or not these two functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, but then the set_stream ops doesn't make anymore sense and I think that normally it is here to let the application indicates that the HW can start processing the data now.
Normally, it is more like:
- application enqueue the minimum required buffer to properly behave
- application perform the set_stream to configure the HW to start the processing
 Indeed I agree that here there isn't much context but I still think that this is a point that the application shouldn't to care about, I mean from the application point of view the API behavior should remain the same.
Also, the start of the processing does not happen only on the enqueue but as well at the end of frame N, if a new frame can be encoded (aka the application has already queue enough for that), then it will process the next one as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but still some questions for a small amount of minor details.
Thanks for adding support for a video encoder, good for experimenting with M2M APIs.
| { | ||
| if (caps->type == VIDEO_BUF_TYPE_OUTPUT) { | ||
| caps->format_caps = stm32_jpeg_out_fmts; | ||
| } else { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If interested in supporting other VIDEO_BUF_TYPE_* in the future, this could be an else if.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments.
|  | ||
| /* JPEG Encoding */ | ||
| jpeg_conf.ColorSpace = JPEG_YCBCR_COLORSPACE; | ||
| jpeg_conf.ChromaSubsampling = conf->subsampling; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May conf be NULL at this point? Maybe it has already been validated by stm32_jpeg_set_fmt(p).
Would __ASSERT_NO_MSG(conf != NULL); make sense here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The set would prevent that. The only reason I can think of is if the default value of the driver get changed during a dev / fix and this is not supported, leading to getting a conf == NULL if the appli decide not to do a set_format (which it could). Hence ok, I add the __ASSERT_NO_MSG for that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we limit the use of __ASSERT_NO_MSG()  as it will / should not go into production code (e.g., need to use it if we are in a void function) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, good point @ngphibang.
Let me change this to use the first entry of the caps structure for default format. That done, the potential error case I pointed in my previous comment cannot happen and then all path are protected and there is no possibility to lead to a NULL ptr here.
e0db8c2    to
    d04dd19      
    Compare
  
    | I also added a new commit at the beginning to change the input/output setting order in the encoder. Indeed, I feel it make sense to ensure that the input is set PRIOR to setting the output since the driver might adapt the output settings after having received the input settings. Does that make sense ? (VENC still work with this modification, and this also already the way I coded it into the UVC encoder sample) | 
| The STM32 JPEG codec can decode JPEG compressed frames | ||
| and encode uncompressed frames to the JPEG format. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I'm not familiar with video conventions: Is it worth specifying that this is an HW accelerator ie that we're not talking about a SW Codec ?
| 
 I think it can be done in either ways so the commit is OK if we see it is more convenient that way in the sample. BTW, I think the drivers should memorize and check the input and output formats so that they are compatible despite of the order. The important thing is we should not enforce the order in the application. (For example, in libMP or gstreamer, caps negotiation can be fixed between the output and the downstream first then between the input and the upstream, or vice versa. Also, during caps negotiation, to transform a caps from one side to another, we also call set_format() on both sides to check if an input/output format can go with the other one) | 
| 
 Hum, in such case I will have to rework the way done now in the last version of the JPEG driver I pushed. If we don't mandate to have first the input, then the output, at set_fmt time few type checking can be done and this needs to be moved to the set_stream time (a bit as I did in the first version). The merit (I think) of forcing the input first is that, whenever setting first the input, the driver will update the output to a valid configuration. Then the application can check if it is ok for the use-case and change it if not. Also set_fmt time check can be done. | 
d04dd19    to
    a2b2a63      
    Compare
  
    | I've pushed a new update of the PR, performing the following modifications while we keep discussing on the way the M2M API should behave. 
 | 
Add description of the ST JPEG HW codec IP node. Signed-off-by: Alain Volmat <[email protected]>
Initial version of the support for the STM32 JPEG HW codec, currently supporting only NV12 to JPEG without DMA support and using SW based conversion from NV12 to MCU required for the JPEG codec. Signed-off-by: Alain Volmat <[email protected]>
Add the node describing the JPEG HW codec within the stm32n6 Signed-off-by: Alain Volmat <[email protected]>
Enable the JPEG HW codec on the stm32n6570_dk board. Signed-off-by: Alain Volmat <[email protected]>
Add conf / overlay files in order to enable the jpeg encoder on the stm32n6570_dk. Signed-off-by: Alain Volmat <[email protected]>
a2b2a63    to
    0e5daa2      
    Compare
  
    | PR simply rebased on top of HEAD to fix merge conflict (due to conflict in the stm32n6_dk_common.dtsi) | 
| 
 | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing blocking left I could think of for my side.
It will be interesting to see how format negotiation can happen in the future for M2M and other long pipeline of formats.
| # | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
| # | ||
|  | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As @JarmouniA taught me, there is a title: available.
| title: STM32 JPEG hardware codec | 
| # | ||
|  | ||
| description: | | ||
| STM32 JPEG HW Codec. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| STM32 JPEG HW Codec. | 
| struct video_buffer *current_in; | ||
| struct video_buffer *current_out; | ||
|  | ||
| struct video_ctrl jpeg_quality; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be come very convenient to have a struct stm32_jpeg_ctrl by looking at the evolution of video control init (where sizeof(data->ctrls) / sizeof(struct video_ctrl) could be used).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though it is also easy to change this in the PR that modifies the video control init.



The PR adds an initial version of the STM32 JPEG Codec, available on several STM32. This PR adds it within the N6.
While the HW can perform both encoding & decoding, for the time being only encoding is provided.
The HW needs data in MCU format in order to encore into JPEG. Formating of the data into MCU is provided only for the NV12 for the time being (for that purpose probably only NV12 should be kept in this driver in order to avoid confusion).
DMA can also be used in order to inject data into the codec but this is not implemented in this version.
Main purpose of this PR is actually to provide material in order to discuss about a possible abstraction for the m2m buffers.
The driver already provide several structures which embed k_fifo but it also make it clear that m2m helper functions could be possible in order to handle in a generic way checking about in & out buffer, and have the helper call a new m2m codec api which would be in charge to perform the conversion only.
As shown in this code, the queue / dequeue function do not have anything HW specific and also take care of pushing / pulling into / from the right fifo and triggering a codec run if all condition are matched.
I hope this could generate discussion about this and based on that I can provide some helpers to hide the non HW specific code (buffer handling).
This has been tested using the tcpclientsink application modified within the PR #95862.
I however had to perform some more changes into the application, especially in order to properly set the in / out formats of the encoded video device. I provide those modifications for reference within this commit (avolmat-st@86249bf) part of my git repo.
This has been tested successfully on the STM32N6 connected over network with laptop into which following command is ran in order to grab JPEG data: