[Bug 707361] video: Add support for 64x32 tiled NV12 color format

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=707361
  GStreamer | gst-plugins-base | git

--- Comment #27 from Nicolas Dufresne <nicolas.dufresne at collabora.co.uk> 2013-12-19 14:56:05 UTC ---
(In reply to comment #26)
> Some remarks:
> 
> - You now moved the unpacking of the tiled formats to a higher layer, the frame
> layer. You seem to want to do this so that you can use extra fields (from the
> GstVideoInfo, the x/y tiles) to do the unpacking. Why not place those extra
> fields in the last free plane and avoid this extra layer of unpacking?

Adding an extra plane is more difficult then it looks likes. With the extra
plane, we need to add code all over the stack to be able to ignore it, parse
it, produce it. It is also very error prone, developer would constantly forget
to add it, which result is crash or worst, undefined behaviour. Also, using a
free plane has the problem that it prevent using tiles for certain formats like
A420 (no plane left).

Adding extra field is much cleaner and keep the code well centralized with the
added addition that it is easy to reuse for any tiled formats. The extra layer
is really thin, I've also took the time to profile it, and it is not a visible
overhead.

> 
> - You have implemented a default function to fill the x/y tiles in the
> GstVideoInfo. When you use GstVideoMeta, how is this updated? where does the
> x/y tiles come from? are you going to use the width/height of the GstVideoMeta
> for this? You could also store this in the 4th plane..

There is a call to gst_video_info_align() that will update it. My finding after
more research, is that the un-cropped size can be deduced from the video
alignment. In the case there is not video alignment, a default will be assumed.
This not much different than any other formats. It would have been put into
GstVideoMeta, but it would be an ABI break as there is no padding. I sill think
we should use more the video alignment, as otherwise we have duplicate
information, with risk of error. As an example, current we could put a
different width and height in the VideoMeta then what is in the caps, it's
being copied, and that would have side effect as it's not expected.

> 
> - frame_pack and frame_unpack seem to only be able to unpack one line of a
> frame starting from the leftmost pixel. This seems weird as a general unpack
> function.

You can review the existing implementation, unpack/pack multiple lines or
unpack/pack from a specific offset is not implemented. But I can add it to the
API, if it's there because there is an intention to implement that.

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.