techyian / MMALSharp

C# wrapper to Broadcom's MMAL with an API to the Raspberry Pi camera.
MIT License
195 stars 33 forks source link

TakePicture not setting ImageContext resolution #186

Closed MV10 closed 3 years ago

MV10 commented 3 years ago

While testing convolution changes for PR #175 I noticed that the ImageContext.Resolution is always 0,0 when ApplyConvolution was invoked after calling TakePicture. The data is valid (streaming into Bitmap produces the correct resolution).

techyian commented 3 years ago

Unfortunately, I think this expected behaviour. The image encoder does not have a built-in resize block so in theory, it doesn't make sense to set the resolution against it, but there is no side-effect to doing so. A solution to this problem would be to edit the Resolution properties against StillPort and VideoPort to look like this:

private Resolution _resolution;

/// <inheritdoc />
public override Resolution Resolution
{
    get
    {
        if (_resolution.Width == 0 || _resolution.Height == 0)
        {
            return new Resolution(MMALCameraConfig.Resolution.Pad().Width, MMALCameraConfig.Resolution.Pad().Height);
        }

        return _resolution;
    }

    internal set
    {
        if (value.Width == 0 || value.Height == 0)
        {
            this.Width = MMALCameraConfig.Resolution.Pad().Width;
            this.Height = MMALCameraConfig.Resolution.Pad().Height;
            _resolution = new Resolution(MMALCameraConfig.Resolution.Pad().Width, MMALCameraConfig.Resolution.Pad().Height);
        }
        else
        {
            this.Width = value.Pad().Width;
            this.Height = value.Pad().Height;
            _resolution = new Resolution(value.Pad().Width, value.Pad().Height);
        }                                
    }
}

I think this should fix it. There should also be a change to the Width and Height properties stored in PortBase making them public properties so the user can query the resolution directly from the port, but make the setter internal to prevent direct modification. I'm also tempted to rename these properties to NativeWidth and NativeHeight. This would also need a change to the IPortBase interface to include them:

public int NativeWidth
{
    get => this.Ptr->Format->Es->Video.Width;
    internal set => this.Ptr->Format->Es->Video.Width = value;
}

public int NativeHeight
{
    get => this.Ptr->Format->Es->Video.Height;
    internal set => this.Ptr->Format->Es->Video.Height = value;
}

Please let me know your thoughts. Happy to get this pushed in.

MV10 commented 3 years ago

I honestly don't have enough experience with still captures to express an opinion either way.

Coming at this as a pure library consumer, I would simply expect the ImageContext fields to always be reliable.

I'm also tempted to rename these properties to NativeWidth and NativeHeight.

Sorry, I don't follow this either. Would these be the padded buffer dimensions versus the configured resolution?

MV10 commented 3 years ago

There should also be a change to the Width and Height properties stored in PortBase making them public properties so the user can query the resolution directly from the port

Just a note to say that wouldn't have fixed the problem I had trying to retrieve the resolution for effects processing -- the Processing library doesn't have a reference back to the core MMALSharp library (otherwise I would have tried to read the resolution from MMALCameraConfig). For that type of scenario it has to wind up in ImageContext, it's all we have to work from.

techyian commented 3 years ago

Yes, my comment about the Width and Height properties was somewhat unrelated to your issue but needed factoring in due to changing the Resolution properties. Currently, the Resolution properties query the width/height directly from the port, but my change steers away from that and will return what the user has stored or what is in MMALCameraConfig.