Hi Gwenole,
   I see you add a <source> argument for simple-encoder. I think maybe some black/white/blue/red... pictures(or blocks) generated by the simple-encoder should be enough. Otherwise, gst-vaapi package may need to have a Y4M file for user to test. Simple-encoder is only for unit tests not to encoding real file.

Thanks,
Wind

Hi,

(In reply to comment #1)

>    I see you add a <source> argument for simple-encoder. I think maybe some
> black/white/blue/red... pictures(or blocks) generated by the simple-encoder
> should be enough. Otherwise, gst-vaapi package may need to have a Y4M file for
> user to test. Simple-encoder is only for unit tests not to encoding real file.

simple-encoder test program is meant to encode real files too, i.e. show how to use libgstvaapi APIs, not simply for unit testing. Unit tests are in test-* sources. We probably could have a test-encode program to encode a single frame too, but the most useful thing is to have that simple-encoder. It will also serve performance evaluation and tuning purposes.

Besides, I opted for Y4M source format because it's very trivial to implement and generate. It should be less than 70 lines with comments.

I thought it was only a simple test-encode. where you hope the code can be put, still in test directory?
even Y4M, we need only support I420 format, right?

(In reply to comment #3)
> I thought it was only a simple test-encode. where you hope the code can be put,
> still in test directory?

Yes, alongside with simple-decoder there. We probably could move that to some tools/ directory, but tests/ was convenient so that to share some common files.

> even Y4M, we need only support I420 format, right?

Yes. Thanks.

Created attachment 264111 [details] [review]
0001-add-a-simple-encoder-test-program-that-uses-libgstva

Hi Gwenole,
   Changzhi <changzhix.wei@intel.com> has wrote a patch for the simple encoder. please review, anything, please comment.

Thanks,
Wind

Created attachment 264522 [details] [review]
free surface queue

1.free surface queue 
2.if there is no source file, auto generated a image, after encode ,save to test.h264/test.mpeg2

Review of attachment 264111 [details] [review]:

Overall:
- Squash patch two into patch one ;
- Use gst-indent(*) over any new file ;
- Remove auto-generation of VA surface contents. If no filename is supplied, <stdin> could be used instead so that you can have y4mcolorbars | simple-encoder [...].
- Update the code to match current libgstvaapi APIs.

(*) The tool is available from any upstream gstreamer git master repo in common/ submodule.

::: tests/simple-encoder.c
@@ +114,3 @@
+    GstVaapiEncObjUserDataHead head;
+    GstBuffer *vaapi_buf;
+} GstVaapiEncodeFrameUserData;

This is not needed. You always get a GstVaapiCodedBufferProxy from gst_vaapi_encoder_get_buffer_with_timeout().

@@ +311,3 @@
+
+    vaMapBuffer(va_dpy,surface_image.buf,(void **)&surface_p);
+

Always use libgstvaapi APIs. Direct usage of VA-API calls is needed and not desired.

::: tests/y4mreader.c
@@ +1,3 @@
+#include "y4mreader.h"
+
+#define MAX_YUV4_HEADER 80

Not really useful?

@@ +4,3 @@
+#define Y4M_MAGIC "YUV4MPEG2"
+#define Y4M_FRAME_MAGIC "FRAME"
+

struct _Y4MImageFile {
  FILE *fp;
  guchar *header;
  guint header_size;
  Y4MImageFileInfo info;
};

static gboolean
read_header(Y4MImageFile *file)
{
}

static gboolean
parse_header(Y4MImageFile *file)
{
}

@@ +5,3 @@
+#define Y4M_FRAME_MAGIC "FRAME"
+
+int y4m_read_head(FILE* stream,y4m_info *y4minfo)

Y4MImageFile *
y4m_image_file_open(const gchar *filename)
{
}

The function will:
- Allocate Y4MImageFile structure ;
- Open the file with supplied name. Or stdin if NULL ;
- Call read_header() to fill up the header data ;
- Call parse_header() to parse the header (Y4MImageFileInfo).

@@ +34,3 @@
+        switch (*tokstart++) {
+        case 'W': // Width. Required.
+            width    = strtol(tokstart, &tokend, 10);

Correct way to read unsigned int would normally be:

unsigned long v = strtoul(tokstart, &tokend, 10);
if (*tokstart && *tokend == '\0')
  <value> e.g. info->width = v;
else {
  // error out
}

@@ +43,3 @@
+        case 'C': // Color space
+            if (strncmp("420", tokstart, 3) == 0) {
+                info.format = 1;

info->format = GST_VIDEO_FORMAT_I420;

@@ +66,3 @@
+            break;
+        case 'F': // Frame rate
+            sscanf(tokstart, "%d:%d", &raten, &rated); // 0:0 if unknown

if (sscanf(...) != 2)
   // error out

@@ +107,3 @@
+    if(current_location>0)
+        frame_count = (filelength-current_location)/one_frame_length;
+    info.framecount = frame_count;

The frame count calculation is not deterministic. There could be parameters after the "FRAME" token. Just drop it.

@@ +112,3 @@
+}
+
+int y4m_locate_first_frame(FILE* stream)

This won't be needed now that, on return from y4m_image_file_open(), you'd already be at the first "FRAME" token.

@@ +135,3 @@
+}
+
+int y4m_read_one_frame(FILE* stream,char**yuvdata,int size)

gboolean
y4m_image_file_read_frame(Y4MImageFile *file, GstVaapiImage *image, GError **out_error_ptr)
{
  // the user allocated GstVaapiImage is filled up, plane by plane to cope with stride requirements from GstVaapiImage.
}

::: tests/y4mreader.h
@@ +1,3 @@
+#include <stdlib.h>
+#include <stdio.h>
+#include <sys/stat.h>

This header, <sys/stat.h> is not needed.

@@ +15,3 @@
+int y4m_read_head(FILE* stream,y4m_info *info);
+int y4m_locate_first_frame(FILE* stream);
+int y4m_read_one_frame(FILE* stream,char**yuvdata,int size);

typedef struct _Y4MImageFile Y4MImageFile;
typedef struct _Y4MImageFileInfo Y4MImageFileInfo;

typedef struct _Y4MImageFileInfo {
  GstVideoFormat format;
  guint width;
  guint height;
  gfloat fps;
  guint flags; // a combination of GST_VAAPI_PICTURE_STRUCTURE ("mixed" not supported yet)
};

Y4MImageFile *
y4m_image_file_open(const gchar *filename);

void
y4m_image_file_close(Y4MImageFile *file);

gboolean
y4m_image_file_get_info(Y4MImageFile *file, Y4MImageFileInfo *out_info_ptr);

gboolean
y4m_image_file_read_image(Y4ImageFile *file, GstVaapiImage *image, GError **out_error_ptr);

Review of attachment 264111 [details] [review]:

::: tests/simple-encoder.c
@@ +30,3 @@
+#include <gst/vaapi/gstvaapiimage_priv.h>
+#include <gst/vaapi/gstvaapiencoder_mpeg2_priv.h>
+#include <gst/vaapi/gstvaapiencoder_h264_priv.h>

Never use private definitions.

@@ +67,3 @@
+    g_cond_signal (&(GST_VAAPI_ENCODER_CAST(encoder))->codedbuf_free);  \
+  } G_STMT_END
+

Use your own locks, not GstVaapiEncoder private definitions.

@@ +588,3 @@
+    free(yuvdata);
+}
+

How to stream Y4M frames into GstVaapiSurface.

Initially:
- Create a GstVaapiSurfacePool, based on information from Y4MImageFileInfo (format, width, height -> gst_video_info_set_format() with format forced to GST_VIDEO_FORMAT_ENCODED).
- Create a GstVaapiImage with specified format, width, height.

In the producer loop, i.e. image reader loop:
- Create a GstVaapiSurfaceProxy from the GstVaapiSurfacePool
- Read a frame from the Y4M file. If returning FALSE, then stop image reader thread
- Upload the GstVaapiImage to the GstVaapiSurface

@@ +742,3 @@
+        g_codec_str = (char*)malloc (5);
+        memset(g_codec_str,0,5);
+        memcpy(g_codec_str,"h264",4);

g_codec_str = g_strdup("h264");

Created attachment 265214 [details] [review]
the patch of simple-encoder

Hi Gwenole:

   I modified the code according to your suggestion, and the file is based on your newest code.

Thanks,
Wei,changzhi

Extremely sorry for the long delay ! I will push this by next week, after 0.5.10 release..

I tried to merge this patch last Friday but it is quite obsolete. It doesn't compile anymore.

I'll try to make it work and learn a bit about the encoding.

Created attachment 302725 [details] [review]
tests: add simple-encoder program

This patch adds a simple-encoder test program that uses libgstvaapi for video encoding
to elementary (raw) streams. Input stream is raw YUV in the Y4M format. That can be
from a regular file or standard input when the input filename is "-".

Usage: simple-encoder [options]* <source>

Options:
--output|-o     output file name
--codec|-c      codec to use for video encoding
--bitrate|-b    desired bitrate (kbps)

By default, and as an initial patch, the encoded stream shall conform to the
minimally supported profile. That is "Constrained Baseline Profile" for H.264
and "Simple Profile" for MPEG-2. Though, those are the defaults to be generated
by libgstvaapi.

Comment on attachment 302725 [details] [review]
tests: add simple-encoder program

Oops! This patch was a mistake. Sorry for the noise.

Created attachment 302920 [details] [review]
tests: add simple-encoder program

This patch adds a simple-encoder test program that uses libgstvaapi for video encoding
to elementary (raw) streams. Input stream is raw YUV in the Y4M format. That can be
from a regular file or standard input when the input filename is "-".

Usage: simple-encoder [options]* <source>

Options:
--output|-o     output file name
--codec|-c      codec to use for video encoding
--bitrate|-b    desired bitrate (kbps)

By default, and as an initial patch, the encoded stream shall conform to the
minimally supported profile. That is "Constrained Baseline Profile" for H.264
and "Simple Profile" for MPEG-2. Though, those are the defaults to be generated
by libgstvaapi.

You can find Y4M sample files here http://samples.mplayerhq.hu/yuv4mpeg2/

Based on changzhix.wei@intel.com's patch.

Review of attachment 302920 [details] [review]:

Add:

   Original-patch-by: Changzhi Wei <changzhix.wei@intel.com>
   [list of changes on top of that]
   s-o-b: you

(not exhaustively reviewed yet).

::: tests/simple-encoder.c
@@ +410,3 @@
+
+  while (1) {
+    if (!load_frame (app, image))

Make load_frame() map, load, unmap. In the future, pushing a VA image that is still mapped could fail.

Review of attachment 302920 [details] [review]:

+ please stop copying the wrong copyright template header. For some time/reason, I had added an extra " " (space) between the the star and the text. :)
i.e. please keep a single whitespace instead of two.

Review of attachment 302920 [details] [review]:

::: tests/Makefile.am
@@ +129,3 @@
+simple_encoder_CFLAGS   = $(TEST_CFLAGS) $(GST_VIDEO_CFLAGS)
+simple_encoder_LDADD    = libutils.la libutils_dec.la $(TEST_LIBS) \
+	$(GST_VIDEO_LIBS)

I don't think libutils_dec to be necessary. However, a libutils_enc.la might be useful to hold e.g. the Y4M file reader.

::: tests/y4mreader.c
@@ +35,3 @@
+  gchar *str;
+
+  memset (header, 0, 80);

Why 80 bytes max all over in here? I don't see that in the specs. Maybe use BUFSIZ?

@@ +89,3 @@
+        guint num = strtol (str, NULL, 0);
+        if (errno)
+          return FALSE;

Same pattern copied all over here, please create a dedicated function to safely parse an int. e.g. static gboolean parse_int(const gchar *str, int *out_value_ptr);

@@ +109,3 @@
+        break;
+      }
+      case 'A': /* sample aspect ration */

ratio.

@@ +208,3 @@
+
+  size_t size = fread (plane, 1, frame_size, file->fp);
+  return size == frame_size;

You will need to split read_frame() into read_plane() and possibly read_line() [inlined?] so that to cope with Y4M image stride != VA image stride.

(In reply to Gwenole Beauchesne from comment #15)
> Review of attachment 302920 [details] [review] [review]:
> 
> Add:
> 
>    Original-patch-by: Changzhi Wei <changzhix.wei@intel.com>
>    [list of changes on top of that]

I would only write those changes in general, because the refactor took me ~95 commits.

(In reply to Gwenole Beauchesne from comment #17)
> Review of attachment 302920 [details] [review] [review]:
> ::: tests/y4mreader.c
> @@ +35,3 @@
> +  gchar *str;
> +
> +  memset (header, 0, 80);
> 
> Why 80 bytes max all over in here? I don't see that in the specs. Maybe use
> BUFSIZ?

I copied from here: https://svn.xiph.org/trunk/theora/examples/encoder_example.c

Created attachment 302966 [details] [review]
tests: add simple-encoder program

This patch adds a simple-encoder test program that uses libgstvaapi for video
encoding to elementary (raw) streams. Input stream is raw YUV in the Y4M
format. That can be from a regular file or standard input when the input
filename is "-".

Usage: simple-encoder [options]* <source>

Options:
--output|-o     output file name
--codec|-c      codec to use for video encoding
--bitrate|-b    desired bitrate (kbps)

By default, and as an initial patch, the encoded stream shall conform to the
minimally supported profile. That is "Constrained Baseline Profile" for H.264
and "Simple Profile" for MPEG-2. Though, those are the defaults to be
generated by libgstvaapi.

You can find Y4M sample files here http://samples.mplayerhq.hu/yuv4mpeg2/

Original-patch-by: Changzhi Wei <changzhix.wei@intel.com>
* general code clean-up
* removed the yuv reader thread
* re-wrote the y4m file parser
* updated used API fixed some wrong usage
* fixed a lot of memory leaks
* added the bitrate setting
* keep fps' numerator and denominator
* simplified the thread control
* removed custom logging and use glib

Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>

Created attachment 302967 [details] [review]
tests: add simple-encoder program

This patch adds a simple-encoder test program that uses libgstvaapi for video
encoding to elementary (raw) streams. Input stream is raw YUV in the Y4M
format. That can be from a regular file or standard input when the input
filename is "-".

Usage: simple-encoder [options]* <source>

Options:
--output|-o     output file name
--codec|-c      codec to use for video encoding
--bitrate|-b    desired bitrate (kbps)

By default, and as an initial patch, the encoded stream shall conform to the
minimally supported profile. That is "Constrained Baseline Profile" for H.264
and "Simple Profile" for MPEG-2. Though, those are the defaults to be
generated by libgstvaapi.

You can find Y4M sample files here http://samples.mplayerhq.hu/yuv4mpeg2/

Original-patch-by: Changzhi Wei <changzhix.wei@intel.com>
* general code clean-up
* removed the yuv reader thread
* re-wrote the y4m file parser
* updated used API fixed some wrong usage
* fixed a lot of memory leaks
* added the bitrate setting
* keep fps' numerator and denominator
* simplified the thread control
* removed custom logging and use glib

Signed-off-by: Víctor Manuel Jáquez Leal <victorx.jaquez@intel.com>

Gwenole, do you approve the patch? :)

Attachment 302967 [details] pushed as be40a1d - tests: add simple-encoder program