GNOME Bugzilla – Bug 763006
checksumsink: Add ability to checksum video planes
Last modified: 2018-11-03 13:47:09 UTC
Enhance checksumsink to optionally generate checksums of individual video planes. These per-plane checksums can be used as a validation aid with H.265 streams which contain decoded_picture_hash SEI messages.
Created attachment 322903 [details] [review] [PATCH 1/3] debugutils: checksumsink: Make checksum type a property
Created attachment 322905 [details] [review] [PATCH 2/3] debugutils: checksumsink: Add option to checksum video planes individually
Created attachment 322906 [details] [review] [PATCH 3/3] debugutils: checksumsink: Support GstVideoCropMeta
Created attachment 323014 [details] [review] [PATCH v2 2/3] debugutils: checksumsink: Add option to checksum video planes individually
Created attachment 323015 [details] [review] [PATCH v2 3/3] debugutils: checksumsink: Support GstVideoCropMeta
Review of attachment 323015 [details] [review]: Looks like a good idea, though I'm not sure about the property. Why do we need a property ?
(In reply to Nicolas Dufresne (stormer) from comment #6) > Review of attachment 323015 [details] [review] [review]: > > Looks like a good idea, though I'm not sure about the property. Why do we > need a property ? I was thinking that it would be able to choose multiple ways to checksum the input that you could choose with a property. Do you think it would be better to just always checksum the video planes if video is given? Or maybe to put this into a separate element?
My thinking was more that doing a checksum of the data outside the crop area is useless. Maybe you have a valid use case for that ? Note, if you don't support CropMeta, the data will be copied before being sent to this sink.
(In reply to Nicolas Dufresne (stormer) from comment #8) > My thinking was more that doing a checksum of the data outside the crop area > is useless. Maybe you have a valid use case for that ? Note, if you don't > support CropMeta, the data will be copied before being sent to this sink. Ah, right. So the H.265 decoded_picture_hash case is the one where I need to disregard crop data and checksum the whole original frame. From what you're saying though it sounds like that shouldn't really work for me, but it currently does with gstreamer-vaapi. If I understand correctly it sounds like vaapidecode should be making a copy of just the cropped area if the CropMeta isn't supported instead of its current behavior of sending the full frame.
Yes, gstreamer-vaapi miss-behave when VideoMeta or VideoCropMeta is missing. I have told Victor recently. The -base libraries are missing few helpers to help with that (for the crop). The point here, is that there is very little use for calculating the checksum over a portion of the image that falls outside of the crop area, as the values in that portion are not significant. Its like doing the checksum ignoring the VideoMeta.
Created attachment 323024 [details] [review] [PATCH v3 3/3] debugutils: checksumsink: Support GstVideoCropMeta (In reply to Nicolas Dufresne (stormer) from comment #10) > Yes, gstreamer-vaapi miss-behave when VideoMeta or VideoCropMeta is missing. > I have told Victor recently. The -base libraries are missing few helpers to > help with that (for the crop). The point here, is that there is very little > use for calculating the checksum over a portion of the image that falls > outside of the crop area, as the values in that portion are not significant. > Its like doing the checksum ignoring the VideoMeta. Yep, understood. Here's an updated patch which removes that useless property.
Comment on attachment 322903 [details] [review] [PATCH 1/3] debugutils: checksumsink: Make checksum type a property Thanks, pushed this with minor changes (edits to the enum descriptions): commit 2830145a7121d3c464a82be356d0fe33e3164dd4 Author: Scott D Phillips <scott.d.phillips@intel.com> Date: Tue Mar 1 14:54:01 2016 -0800 checksumsink: add "hash" property and allow more checksum types Now any GChecksumType can be used by GstChecksumSink, adding support for MD5, SHA-256 and SHA-512 in addition to SHA-1. https://bugzilla.gnome.org/show_bug.cgi?id=763006
I'm a bit undecided about the other video-specific patches, I'm wondering if it would make sense to add a video-specific subclass/variant instead of adding extra video-specific stuff to this one. On a side note, I dislike the fact that the element prints things to stdout instead of posting messages with the data on the bus or such. I'm guessing you just pipe the output to stdout and then diff against the expected output?
Created attachment 326973 [details] [review] [PATCH] debugutils: create the `videospy` element Hi Tim, thanks for the review. I notice that right next to the checksumsink element is the debugspy element that posts messages to the bus instead of printing to stdout. Here's a patch that makes a new element 'videospy' derived from debugspy that posts per-plane checksums to the bus.
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/issues/354.