After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 729764 - pnmdec: Not respecting downstream rowstride
pnmdec: Not respecting downstream rowstride
Status: RESOLVED NOTABUG
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal normal
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-05-08 03:55 UTC by Sanjay NM
Modified: 2015-02-05 11:56 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
pngdec: Fix to decode image into original dimension (3.32 KB, patch)
2014-05-08 03:55 UTC, Sanjay NM
needs-work Details | Review
pngdec: Patch to get pnm decoder output without stride alignment (4.20 KB, patch)
2014-05-21 05:17 UTC, Sanjay NM
needs-work Details | Review

Description Sanjay NM 2014-05-08 03:55:59 UTC
Created attachment 276118 [details] [review]
pngdec: Fix to decode image into original dimension

PNM Decoder always aligns the output dimension to multiple of 4. There is no way to say decode an image of dimension 1919 x 1080 into the same buffer size. This gets decoded to 1920 x 1080 size.

Fixed this by adding an input option to decode the image into original dimension. Default functionality is not changed.
Comment 1 Sanjay NM 2014-05-08 04:09:49 UTC
Tested it by below pipeline

gst-launch-1.0 filesrc location=PNMTest.pnm ! pnmdec samedimension=TRUE ! filesink location=out.bin
Comment 2 Thiago Sousa Santos 2014-05-13 23:38:56 UTC
Review of attachment 276118 [details] [review]:

While this patch doesn't address an issue, there is room for improvement here in pnmdec. What needs to be done is check if downstream has a 'rowstride' field in its caps and use that. Otherwise just use rowstride = width.

Another thing I noticed is that it is using buffer appends until it gets a full frame, it would be more resilient if it used a GstAdapter and only retrieved a buffer of an expected size.

::: gst/pnm/gstpnmdec.c
@@ +106,3 @@
 {
   /* Need to convert from PNM rowstride to GStreamer rowstride */
+  if ((s->mngr.info.width % 4 != 0) && (s->dimension == FALSE)) {

This is actually only increasing the image to have a rowstride that is a multiple of 4. The final buffer caps is still of the input width and should work as expected.

I don't think we have a restriction of having a multiple of 4 rowstride, it should actually be respecting whatever downstream can do if it provides a rowstride value in the caps.
Comment 3 Sanjay NM 2014-05-21 05:17:48 UTC
Created attachment 276915 [details] [review]
pngdec: Patch to get pnm decoder output without stride alignment

At present pnmdec always aligns rowstride to 4 and then returns output buffer. This patch is to get the output without any rowstride alignment
Comment 4 Sanjay NM 2014-05-21 05:19:12 UTC
Thanks for your review. At present pnmdec always aligns rowstride to 4 and then returns this buffer. If width is say 1919, considering bits per pixel of 3 rowstride becomes 1919 * 3 = 5757. This at present is getting adjusted to 5760 (multiple of 4) and then buffer gets returned. Sorry for confusing by saying width becomes 1920 (5760/4 = 1920). I have updated the patch to take an input parameter rowstrideorig. If this is set to TRUE then pnmdec will not align rowstride to 4 but will return the original (width * bpp) number of bytes.
If no parameter is passed then the behavior is same as before. That is it will make the rowstride alignment to 4 and return. This is more of an enhancement.

I have tested the decoder by using following commands and it is working well.

gst-launch-1.0 filesrc location=odd.pnm ! pnmdec rowstrideorig=TRUE ! filesink location=out.raw

gst-launch-1.0 filesrc location=odd.pnm ! pnmdec ! filesink location=out.raw

I will create separate patches for your suggestions to use GstAdapter.
Comment 5 Thiago Sousa Santos 2014-05-21 14:28:12 UTC
Review of attachment 276915 [details] [review]:

Please don't run gst-indent for .h files, only .c needs to be properly indented.

See inline comments below. Please look at the code I pointed in the comments below so you understand why this caps negotiation is important and how it works. This can also help understanding: http://cgit.freedesktop.org/gstreamer/gstreamer/tree/docs/design/part-negotiation.txt

::: gst/pnm/gstpnmdec.c
@@ +49,3 @@
+{
+  GST_PNMDEC_PROP_0,
+  GST_PNMDEC_PROP_STRIDE

You shouldn't need a property for this, it should be done automatically: If downstream requests a particular rowstride than this element will provide it. Otherwise it should fallback to either rowstride=width or using a multiple of 4 to maintain backwards compatibility.

@@ +105,3 @@
 {
   /* Need to convert from PNM rowstride to GStreamer rowstride */
+  if ((s->mngr.info.width % 4 != 0) && (s->rowstride == FALSE)) {

This should not be controlled by a property, but from a variable read at the beginning of the chain function (that updates every time a reconfigure event is sent).

You can look at the "ensure_negotiated" function from gstgoom.c in gst-plugins-good for an example. It will check if needs to reconfigure and then queries downstream caps and checks for the properties it needs to know. For pnmdec it would need to check that width and height match and also look if there is any rowstride field to use that instead of always using a multiple of 4.
Comment 6 Sanjay NM 2015-02-05 11:56:15 UTC
This is the way data is handled in gstreamer. I did not have the understanding then. So closing it.