GNOME Bugzilla – Bug 730870
androidmedia: hw decoder doesn't work properly on mtk 8125
Last modified: 2018-11-03 13:23:43 UTC
Created attachment 277364 [details] [review] [PATCH 1/3] androidmedia: correct framesize for COLOR_QCOM_FormatYUV420PackedSemiPlanar64x32Tile2m8ka mtk 8125 has a special tile style color format, where tile size is 16 * 32.
Created attachment 277365 [details] [review] [PATCH 2/3] androidmedia: always tile-by-tile copying for tile color format
Created attachment 277366 [details] [review] [PATCH 3/3] androidmedia: add COLOR_MTK_FormatYUV420PackedSemiPlanar16x32Tile
We should remove all this de-tiling code from androidmedia and instead use the tiled format support from libgstvideo (and add this MTK format there).
++ And add Tile -> NV12 fast path.
Review of attachment 277365 [details] [review]: Seems odd to silently remove something that look like a fair optimization. Maybe some explanation in your commit log could help reviewers.
Review of attachment 277366 [details] [review]: Also, a quick research shows that this code has "Rafaël Carré <funman@videolanorg>" has author, and you use an anonymous name (are you Rafaël ?). When copy pasting code, first you should test and compile it, validate the licence (in this case it's LGPL, so it ok) then you should give original author credits, and for patches submitted to GStreamer, you should use your real full name in the From field. ::: sys/androidmedia/gstamc.c @@ +2750,3 @@ + vptr = v_luma + luma_idx; + cptr = c_luma; + memcpy (*dest, *src, tile_width); Something is wrong here I think. vptr and cptr are updated but unused. And dest/src is undefined.
Ok, I did missed something, it's a copy of the code above, which is indeed from Rafaël as stated in the copyright. But the rest is valid. Anyway, as said, we need to get rid of this mess in gstamc, and only keep the part that make sense. Converting in the decoder is bad, as it will prevent any zero copy if the format is supported lower in the pipeline.
(In reply to comment #3) > We should remove all this de-tiling code from androidmedia and instead use the > tiled format support from libgstvideo (and add this MTK format there). So the COLOR_QCOM_FormatYUV420PackedSemiPlanar64x32Tile2m8ka converting logic should also be moved to libgstvideo? What about other "non de-tiling" converting logic? Like COLOR_FormatYUV420SemiPlanar -> GST_VIDEO_FORMAT_NV12?
(In reply to comment #8) > (In reply to comment #3) > > We should remove all this de-tiling code from androidmedia and instead use the > > tiled format support from libgstvideo (and add this MTK format there). > > So the COLOR_QCOM_FormatYUV420PackedSemiPlanar64x32Tile2m8ka converting logic > should also be moved to libgstvideo? Yes, this one match NV12_64Z32 format. Which is 64x32 tiled format using a Z flip Z layout. > > What about other "non de-tiling" converting logic? Like > COLOR_FormatYUV420SemiPlanar -> GST_VIDEO_FORMAT_NV12? They need to be looked at. Most of them can be expressed in GST format, most of this code was the fast path to working GST, rather then the correct way.
Any progress here?
(In reply to Sebastian Dröge (slomo) from comment #10) > Any progress here? Go through the thread, it suggests this "tile by tile copying" should be implemented in libgstvideo (by adding this MTK format). We were quite in a rush when this patch was written, hence haven't moved it to libgstvideo. After that, I find a new job - neither having the MTK table at hand, nor having much time to hack gstreamer, sorry... As now we have "zerocopy rendering"(https://bugzilla.gnome.org/show_bug.cgi?id=731204), gstamcdec can do the playback without any converting, could I close this bug?
-- 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/150.