GNOME Bugzilla – Bug 694068
h264parser: Parse the cropping-rectangle separately.
Last modified: 2018-11-03 13:14:30 UTC
Created attachment 236561 [details] [review] h264parser: Parse the cropping-rectangle separately. The h264parser should parse the cropping rectangle separately. Because we have to pass the cropping rectangle to the renderer instead of using the cropped-values for decoding.Which means the width and height of SPS header will be un-cropped dimensions. example case: many of the 1920x1088 samples of h264 have 8 padding lines to make the height as a multiple of 16. The actual picture dimension will be 1920x1080.
Any comments, suggestions???
ping ?
Review of attachment 236561 [details] [review]: This will need a change in h264parse if I understand correctly, as h264parse currently uses sps->width/height for the caps. Instead it should now use the cropped width/height. Otherwise looks good to me, will push after the corresponding h264parse change.
(In reply to comment #3) > Review of attachment 236561 [details] [review]: > > This will need a change in h264parse if I understand correctly, as h264parse > currently uses sps->width/height for the caps. Instead it should now use the > cropped width/height. > > Otherwise looks good to me, will push after the corresponding h264parse change. The patch is setting the un_cropped values to sps->width/sps->height. We are supposed to set the cropping values as videometa (instead of setting the cropped values to caps) since it is the duty of renderer to deal with cropping. right?
(In reply to comment #4) > (In reply to comment #3) > > Review of attachment 236561 [details] [review] [details]: > > > > This will need a change in h264parse if I understand correctly, as h264parse > > currently uses sps->width/height for the caps. Instead it should now use the > > cropped width/height. > > > > Otherwise looks good to me, will push after the corresponding h264parse change. > > The patch is setting the un_cropped values to sps->width/sps->height. We are > supposed to set the cropping values as videometa (instead of setting the > cropped values to caps) since it is the duty of renderer to deal with cropping. > right? In my opinion, sps->{width,height} should represent the un-cropped values to be used for allocating buffers. At least, this is how it used today. The actual rendering region needs to be conveyed through another means, and that's videometa.
Hi sree, (In reply to comment #4) > (In reply to comment #3) > > Review of attachment 236561 [details] [review] [details]: > > > > This will need a change in h264parse if I understand correctly, as h264parse > > currently uses sps->width/height for the caps. Instead it should now use the > > cropped width/height. > > > > Otherwise looks good to me, will push after the corresponding h264parse change. > > The patch is setting the un_cropped values to sps->width/sps->height. We are > supposed to set the cropping values as videometa (instead of setting the > cropped values to caps) since it is the duty of renderer to deal with cropping. > right? Do you have the patch for slomo's request? I think we should at least maintain the current semantics to get this patch committed and continue discussions for what h264parse should use. :) At least, we should get the video parsers behave consistently. e.g. if mpegvideoparse is using sequence_display_extension to arrange the caps, if this is smaller than the size reported in sequence header, then for h264parse we probably should adjust the caps based on the frame cropping info. In any case, your current patch already addresses two issues: - this really fills in sps->frame_cropping_flag as we ought to ; - we can determine the cropping rectangle that was parsed.
Created attachment 248625 [details] [review] codecparsers: h264: fix calculation of the frame cropping rectangle Hi, the frame cropping height was mis-calculated. In particular, the frame_crop_top_offset was not scaled up with SubHeightC. While we are at it, I also refactored the code to use variable names from (7-18) to (7-21). i.e. CropUnitX and CropUnitY.
Note: a similar fix needs to be backported to 1.0 branch too, unless we backport the whole video cropping changes. This fixes Base_Ext_Main_profiles/CVFC1_Sony_C.jsv
So this now still puts the display width/height (i.e. after cropping) in the caps via h264parse?
(In reply to comment #9) > So this now still puts the display width/height (i.e. after cropping) in the > caps via h264parse? No, this still requires an additional patch. The patch I posted is needed against sree's patch on master, but also against 1.0-branch without sree's patch. i.e. that's needed in any case, the cropped width was mis-calculated. :) If sree doesn't provide a patch for h264parse for master on top of his initial patch, I will probably do it today unless I get trapped.
Created attachment 248698 [details] [review] h264parser: fix size caps to report cropped dimensions Hi, this patch restores the original h264parse behaviour, that is report the cropped dimensions in size caps.
(In reply to comment #9) > So this now still puts the display width/height (i.e. after cropping) in the > caps via h264parse? To recap, This patche series (3 patches) provides: - Un-cropped size in sps->width/height, as expected for buffer allocation ; - Fix the calculation of the cropped width ; - Make sure that h264parse provides cropped dimensions in size caps as it used to do so before. However, we clearly have an inconsistency between the mpegvideoparse and h264parse elements. And this is not new. The mpegvideoparse element provides uncropped dimensions in size caps, whereas the h264parse element provides cropped dimensions in size caps. So, I suggest we open a new bug report for this discrepancy, unless we can live with it. In any case, the suggested patch series here now maintains the original h264parse behaviour, while extending the codec parser to provide the correct cropped dimensions in separate fields, and the original (uncropped) dimensions in plain width/height fields. Thanks.
Created attachment 248700 [details] [review] h264parse: Use the crop-rectangle dimensions to update src caps. If the encoded stream has an associated crop-rectangle, then use the crop values for updating the src caps (not overriding the upstream values if any). This is still a hack to maintain the current semantics. Fixme: Add GstVideoCropMeta to supply the crop rectangle to downstream elements.
Aha, collision again ;)same patch...
Caps should always contain effective/cropped dimensions, so please open bugs against other components where that's not the case (and can be fixed).
(In reply to comment #15) > Caps should always contain effective/cropped dimensions, so please open bugs > against other components where that's not the case (and can be fixed). Hm, are we supposed to provide the crop-values as GstVideoCropMeta? I think it is the duty of renderer to handle the crop rectangle operation since it is a vpp operation.
@Gwenole: please make my last patch as obsolete since it is the copy of your previous patch :)
(In reply to comment #16) > (In reply to comment #15) > > Caps should always contain effective/cropped dimensions, so please open bugs > > against other components where that's not the case (and can be fixed). > > Hm, are we supposed to provide the crop-values as GstVideoCropMeta? > I think it is the duty of renderer to handle the crop rectangle operation since > it is a vpp operation. I think something like that, yes. The GstVideoCropMeta could be handled by the decoder then, or passed further downstream to be handled by the sink or something else.
commit 18984f98dd0d4894957562894f293fdc3695d9ca Author: Gwenole Beauchesne <gwenole.beauchesne@intel.com> Date: Tue Jul 9 11:39:46 2013 +0200 h264parser: fix size caps to report cropped dimensions. Restore the original h264parser behaviour to report cropped dimensions in size caps. https://bugzilla.gnome.org/show_bug.cgi?id=694068 Signed-off-by: Gwenole Beauchesne <gwenole.beauchesne@intel.com> commit ac9c1ae5a8ff77c3455832ae726fa6a7f7891a4d Author: Gwenole Beauchesne <gwenole.beauchesne@intel.com> Date: Mon Jul 8 18:44:24 2013 +0200 codecparsers: h264: fix calculation of the frame cropping rectangle. Fix calculation of the frame cropping rectangle, and more precisely the actual cropped height. The frame_crop_top_offset subtraction was not scaled up with SubHeightC. Also clean-up variables to align more with (7-18) to (7-21). Signed-off-by: Gwenole Beauchesne <gwenole.beauchesne@intel.com> commit 529ece338fcefd8291e09d6d3fe66357df13078f Author: Sreerenj Balachandran <sreerenj.balachandran@intel.com> Date: Fri Feb 15 14:18:49 2013 +0200 h264parser: Parse the cropping-rectangle separately. Assign the un-cropped width/height to sps->width/sps->height during sps header parsing. Added new fields to SPS header structure to provide the crop-rectangle dimensions. https://bugzilla.gnome.org/show_bug.cgi?id=694068
Created attachment 248706 [details] [review] h264parse: Add GstVideoCropMeta If the encoded stream has any associated crop rectangle, then just pass it to the downstream as GstVideoCropMeta.
Created attachment 248707 [details] Sample file with crop_rectangle This h264 sample has crop_rectangle.
So shall we set the un-cropped values in caps now? :) Similar to mpegvideoparse as Gwenole said before. Do we need to respect the upstream provided value here to update the src_caps (If the upstream is providing cropped values) I didn't create a new bug for this since it is just a continuation of the previous issue. So IMHO it is better to re-open the bug.
If the patch looks fine, then i can provide the patch for mpegvideoparse and vc1parse.
> So shall we set the un-cropped values in caps now? :) Similar to mpegvideoparse > as Gwenole said before. Please see comment #15.
(In reply to comment #24) > > So shall we set the un-cropped values in caps now? :) Similar to mpegvideoparse > > as Gwenole said before. > > Please see comment #15. Hm, Then the bufferpools creating by the downstream elements will only use the cropped values for buffer creation. right? Then what is the usage of GstVideoCropMeta?
The caps in the ALLOCATION query can be different from the caps that were sent downstream in the CAPS event. Decoders using the crop metadata are doing that: the caps in the ALLOCATION query have the uncropped size, the CAPS event caps have the cropped size.
(In reply to comment #26) > The caps in the ALLOCATION query can be different from the caps that were sent > downstream in the CAPS event. Decoders using the crop metadata are doing that: > the caps in the ALLOCATION query have the uncropped size, the CAPS event caps > have the cropped size. Pretty clear :)..Thanks. So what do you think about my VideoCropMeta patch(id=248706) ?
I have created another bug for mpegvideoparse to update the src caps with cropped values if there is a display_extension. https://bugzilla.gnome.org/show_bug.cgi?id=704009
The videocropmeta patch for mpegvideoparse is here: https://bugzilla.gnome.org/show_bug.cgi?id=691712
What should we do here? Is the meya actually passed through elements and used somewhere?
It's going to be passed through GstVideoDecoder nowadays.
What's the status here exactly? Does the crop meta make sense if caps (in events) contain uncropped dimensions already?
Review of attachment 248706 [details] [review]: ::: gst/videoparsers/gsth264parse.c @@ +1730,3 @@ + GST_INFO_OBJECT (h264parse, + "Added CropMeta, crop_rectangle: x=%u y=%u width=%u height=%u", + crop_meta->x, crop_meta->y, crop_meta->width, crop_meta->height); We have no idea if downstream support this or not, is it what we want, best effort ? Meanwhile, we currently read back this information from the decoder itself in general, making this less useful, but would serve if the decoder does not offer it.
-- 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/87.