Created attachment 192226 [details] [review]
0001-backends-Add-comments-about-missing-load_data-implem.patch

I quickly investigated the other backends, but it looks like the poppler backend is the only one that can easily support loading from data. This patch adds comments saying why the others can't do it, so you might like to apply it just to have the information there.

I'm not sure about the comics backend - I don't see yet where the data is actually loaded.

I guess it would be nice to have a way to discover if evince can load a particular mime type as data, rather than having to catch an error asynchronously from EvJob's signal.

Could I get some response, please?

Review of attachment 192170 [details] [review]:

Sorry for the late reply, and thank your for the patch!

::: backend/pdf/ev-poppler.cc
@@ +293,3 @@
+	// See https://bugs.freedesktop.org/show_bug.cgi?id=39322
+	pdf_document->document =
+		poppler_document_new_from_data (const_cast<char*>(data), length, pdf_document->password, &poppler_error);

Even though this file is C++ I prefer to use C style casts.

::: libdocument/ev-document-factory.c
@@ +153,3 @@
+	*compression = EV_COMPRESSION_NONE;
+
+	content_type = g_content_type_guess (NULL, (const guchar*)data, length, 

Would it be possible to create the load job with guchar and use it everywhere to avoid these casts?

@@ +285,3 @@
+
+static void
+free_uncompressed_data (gchar *data)

We don't need this, free_uncompressed_uri is needed because it also removes the temp file, but in this case we can just use g_free directly as GDestroyNotify function

@@ +395,3 @@
+				return document;
+				
+				/* else fall through to slow mime code detection. */

This comment doesn't make sense when loading from data, I guess.

::: libdocument/ev-document.c
@@ +214,3 @@
 
+static void
+ev_document_load_cache (EvDocument  *document,

I prefer something like ev_document_setup_cache or init_cache

@@ +217,3 @@
+		  const char  *uri,
+		  const char  *data,
+		  gsize        data_length)

data and data_length are not used at all in this function. I think we could remove both uri and data parameters, and set the uri only when loading from uri before calling this and setup the syntex parser only the case of uri too, after calling this method.

@@ +344,3 @@
 		 * going to the backends since it requires locks
 		 */
+	        ev_document_load_cache (document, uri, NULL, 0);

So, here this would be:

priv->uri = g_strdup (uri);
ev_document_init_cache(document);
if (_ev_document_support_synctex (document))
.....

@@ +380,1 @@
+	retval = klass->load_data (document, data, length, &err);

klass->load_data can be NULL for backends not supporting it, so we should check it here and maybe even fill an error saying loading from data is not supported by that document.

(In reply to comment #3)
> Review of attachment 192170 [details] [review]:
> 
> Sorry for the late reply, and thank your for the patch!
> 

Taking over here. I adapted both patches to be appliable to current master.

> ::: backend/pdf/ev-poppler.cc
> @@ +293,3 @@
> +    // See https://bugs.freedesktop.org/show_bug.cgi?id=39322
> +    pdf_document->document =
> +        poppler_document_new_from_data (const_cast<char*>(data), length,
> pdf_document->password, &poppler_error);
> 
> Even though this file is C++ I prefer to use C style casts.

Done.

> 
> ::: libdocument/ev-document-factory.c
> @@ +153,3 @@
> +    *compression = EV_COMPRESSION_NONE;
> +
> +    content_type = g_content_type_guess (NULL, (const guchar*)data, length, 
> 
> Would it be possible to create the load job with guchar and use it everywhere
> to avoid these casts?

Casts are unavoidable. We might want guchar*, but there are APIs that take char* (poppler_document_new_from_data).

> @@ +285,3 @@
> +
> +static void
> +free_uncompressed_data (gchar *data)
> 
> We don't need this, free_uncompressed_uri is needed because it also removes the
> temp file, but in this case we can just use g_free directly as GDestroyNotify
> function

Removed.

> @@ +395,3 @@
> +                return document;
> +                
> +                /* else fall through to slow mime code detection. */
> 
> This comment doesn't make sense when loading from data, I guess.

Removed.

> ::: libdocument/ev-document.c
> @@ +214,3 @@
> 
> +static void
> +ev_document_load_cache (EvDocument  *document,
> 
> I prefer something like ev_document_setup_cache or init_cache
> 
> @@ +217,3 @@
> +          const char  *uri,
> +          const char  *data,
> +          gsize        data_length)
> 
> data and data_length are not used at all in this function. I think we could
> remove both uri and data parameters, and set the uri only when loading from uri
> before calling this and setup the syntex parser only the case of uri too, after
> calling this method.
> 
> @@ +344,3 @@
>           * going to the backends since it requires locks
>           */
> +            ev_document_load_cache (document, uri, NULL, 0);
> 
> So, here this would be:
> 
> priv->uri = g_strdup (uri);
> ev_document_init_cache(document);
> if (_ev_document_support_synctex (document))
> .....

Done.

> @@ +380,1 @@
> +    retval = klass->load_data (document, data, length, &err);
> 
> klass->load_data can be NULL for backends not supporting it, so we should check
> it here and maybe even fill an error saying loading from data is not supported
> by that document.

Right. I added EV_DOCUMENT_ERROR_UNSUPPORTED to EvDocumentError for it. I will attach patches in next comments.

Created attachment 212892 [details] [review]
Add loading documents from memory, for now only pdf backend.

Created attachment 212893 [details] [review]
Rest of backends have no loading from memory for now.

I wonder if this is correct way of doing it. I guess that instead of setting it to NULL, we should set it to some short default function that just sets an error and returns FALSE. That way, we do not have to worry about this vfunc being NULL.

It seems you forgot to attach the updated first patch?

I see, you actually obsoleted your own patch, fixed now. I don't see the problem with setting the vfunc to NULL, as long as the only way where it is used has the proper != NULL ward

(In reply to comment #8)
> I see, you actually obsoleted your own patch, fixed now.

Thanks.

> I don't see the
> problem with setting the vfunc to NULL, as long as the only way where it is
> used has the proper != NULL ward

Ok, such ward already exists.

Also, I am going to attach two more patches. First implements compression and decompression of documents in memory and another ones just adds some new functions to documentation (section files).

Created attachment 212949 [details] [review]
Adds compression and decompression functions for documents in memory.

Created attachment 212950 [details] [review]
Adds some newly added functions to documentation.

Created attachment 212963 [details] [review]
Adds loading DJVU documents from memory.

And yet another patch, but highly experimental. Murray, I guess that you will want to use loading from memory in your project. If so, it would be great if you could check if this code works for you.

Review of attachment 212892 [details] [review]:

Patch looks great, there are just a few issues that I've fixed before pushing the patch. Thanks!

::: libdocument/ev-document-factory.c
@@ +154,3 @@
+
+	content_type = g_content_type_guess (NULL, data, length,
+		&content_type_uncertain);

g_content_type_guess accepts NULL for result_uncertain and you are not using content_type_uncertain, so we can just pass NULL.

@@ +161,3 @@
+                                     _("Unknown MIME Type"));
+
+		return NULL;

You are leaking content_type here

@@ +323,3 @@
+ * %EV_DOCUMENT_ERROR_ENCRYPTED.
+ *
+ * Returns: a new #EvDocument, or %NULL.

This is missing transfer full annotation

@@ +326,3 @@
+ */
+EvDocument *
+ev_document_factory_get_document_from_data (const guchar *data, gsize length, GError **error)

We can split this method, moving most of the code into a ev_document_factory_load_data() for consistency with ev_document_factory_load_uri().

@@ +358,3 @@
+	*/
+	data_unc = NULL;
+	data_unc_length = 0;

These are already initialized.

@@ +380,3 @@
+		&err);
+
+	if (result == FALSE) {

We can use an early return here

::: libdocument/ev-document.c
@@ +353,3 @@
+ *
+ * On failure, %FALSE is returned and @error is filled in.
+ * If the document is encrypted, EV_DEFINE_ERROR_ENCRYPTED is returned.

This should be EV_DOCUMENT_ENCRYPTED and with % prefix

@@ +354,3 @@
+ * On failure, %FALSE is returned and @error is filled in.
+ * If the document is encrypted, EV_DEFINE_ERROR_ENCRYPTED is returned.
+ * If the backend cannot load the specific document, EV_DOCUMENT_ERROR_INVALID

%EV_DOCUMENT_ERROR_INVALID

@@ +397,3 @@
+		EvDocumentPrivate *priv = document->priv;
+
+		priv->uri = NULL;

Instance structures are initialized to 0 when allocated by glib, so uri is already NULL.

::: libview/ev-jobs.c
@@ +990,3 @@
+		} else {
+			const guchar *uncompressed_data = g_object_get_data (G_OBJECT (job->document),
+									    "data-uncompressed");

Thias doesn't match what you set in ev-document-factory, it was data_unc-uncompessed

@@ +992,3 @@
+									    "data-uncompressed");
+			gsize uncompressed_data_length = GPOINTER_TO_SIZE (g_object_get_data (G_OBJECT (job->document),
+											      "data-length-uncompressed"));

Ditto.

Review of attachment 212893 [details] [review]:

There's no need to explicitly set to NULL the vfunc on all backends.

Review of attachment 212949 [details] [review]:

I think all compression commands can read input from stdin and send the output to stdout, so in the case of data, we could avoid using temp files.

I discussed this with Carlos on IRC and we agreed that it would be much better to not limit this to in memory data, but instead expose a method to read from a GInputStream (which you can then use with a GMemoryInputStream to just feed it in-memory data). That will also simplify the decompression setup since we can just use a GConverterInputStream (although only the gz converter is in stock glib; but there are patches / standalone files around for bzip2 and xz which we could either slurp into evince or convince the glib devs to adopt).

Comment on attachment 212892 [details] [review]
Add loading documents from memory, for now only pdf backend.

This was backed out, and re-done differently. We now support loading EvDocument from GInputStream and GFile. So for data, just wrap it into a GMemoryInputStream.

Comment on attachment 212963 [details] [review]
Adds loading DJVU documents from memory.

This needs to be adapted to the new backend functions load_stream and load_gfile.

Created attachment 215750 [details] [review]
Adds loading DJVU documents from memory, v2.

The loader using GFile and the one using uri could somehow use GCancellable together with ddjvu_job_t. For now they don't.

Review of attachment 215750 [details] [review]:

Thanks for the patch. I have some comments.

::: backend/djvu/djvu-document.c
@@ +257,3 @@
+djvu_document_load_stream (EvDocument          *document,
+			   GInputStream        *stream,
+			   EvDocumentLoadFlags  flags G_GNUC_UNUSED,

Why the G_GNUC_UNUSED, does it produce a compile warning without that?

@@ +271,3 @@
+
+	if (!doc) {
+		djvu_handle_events (djvu_document, TRUE, &djvu_error);

Is this possible? what error can happen if we have just created the document without giving a filename to decode?

@@ +297,3 @@
+	while ((msg = ddjvu_message_peek (ctx))) {
+		switch (msg->m_any.tag) {
+			case DDJVU_NEWSTREAM: {

I think this while + switch could be simplified by using 

djvu_wait_for_message (djvu_document, DDJVU_NEWSTREAM, &djvu_error);

First message should be a newstream, and first stream is always streamid = 0. After reading the stream then you can do 

djvu_wait_for_message (djvu_document, DDJVU_DOCINFO, &djvu_error);

That would ignore all other streams (that we don't support yet) and wait for docinfo message.

@@ +302,3 @@
+				/* first stream */
+				if (streamid == 0) {
+#define CHUNKSIZE 1024 * 1024

why 1024 * 1024?

@@ +304,3 @@
+#define CHUNKSIZE 1024 * 1024
+					GByteArray* array = g_byte_array_new ();
+					guint8* buffer = g_malloc (CHUNKSIZE);

Would it be possible to alloc this in the stack instead?

@@ +307,3 @@
+					GError* read_error = NULL;
+
+					while (1) {

I think it would be clearer to use a do while

do {
....
} while (read_bytes != 0);

since you are returning in case of error.

@@ +315,3 @@
+
+						if (read_bytes < 0) {
+							if (read_error) {

I think g_input_stream_read always fills the error in case of error.

@@ +332,3 @@
+							return FALSE;
+						} else if (read_bytes) {
+							g_byte_array_append (array,

Why are you caching the data instead of just calling ddjvu_stream_write() for every chunk read?

@@ +339,3 @@
+						}
+					}
+					ddjvu_stream_write (doc,

Do you really need to write all the data in one step?

(In reply to comment #20)
> Review of attachment 215750 [details] [review]:
> 
> Thanks for the patch. I have some comments.
> 
> ::: backend/djvu/djvu-document.c
> @@ +257,3 @@
> +djvu_document_load_stream (EvDocument          *document,
> +               GInputStream        *stream,
> +               EvDocumentLoadFlags  flags G_GNUC_UNUSED,
> 
> Why the G_GNUC_UNUSED, does it produce a compile warning without that?

Just have a habit of using it for C sources. I guess that some warnings are issued, depends on warnings verbosity.

> @@ +271,3 @@
> +
> +    if (!doc) {
> +        djvu_handle_events (djvu_document, TRUE, &djvu_error);
> 
> Is this possible? what error can happen if we have just created the document
> without giving a filename to decode?

I am just following one of general conventions in ddjvuapi.h:
- all functions returning a pointer might return a null pointer.

> @@ +297,3 @@
> +    while ((msg = ddjvu_message_peek (ctx))) {
> +        switch (msg->m_any.tag) {
> +            case DDJVU_NEWSTREAM: {
> 
> I think this while + switch could be simplified by using 
> 
> djvu_wait_for_message (djvu_document, DDJVU_NEWSTREAM, &djvu_error);
> 
> First message should be a newstream, and first stream is always streamid = 0.
> After reading the stream then you can do 
> 
> djvu_wait_for_message (djvu_document, DDJVU_DOCINFO, &djvu_error);
> 
> That would ignore all other streams (that we don't support yet) and wait for
> docinfo message.

Hm, but shouldn't we call ddjvu_stream_close() for every stream, be it supported or not? I mean - newstream messages probably have to be handled as well. Also, the structure of this loop is just a copy of what was proposed in ddjvuapi.h.

> 
> @@ +302,3 @@
> +                /* first stream */
> +                if (streamid == 0) {
> +#define CHUNKSIZE 1024 * 1024
> 
> why 1024 * 1024?
> 

Frankly speaking - no idea. I was wondering what chunk size should I use.

> @@ +304,3 @@
> +#define CHUNKSIZE 1024 * 1024
> +                    GByteArray* array = g_byte_array_new ();
> +                    guint8* buffer = g_malloc (CHUNKSIZE);
> 
> Would it be possible to alloc this in the stack instead?

I alloced it on stack at first when I chose chunk size to be just 1024. But I prefered to avoid allocing one MiB on stack - it is rather scarce resource.

> 
> @@ +307,3 @@
> +                    GError* read_error = NULL;
> +
> +                    while (1) {
> 
> I think it would be clearer to use a do while
> 
> do {
> ....
> } while (read_bytes != 0);
> 
> since you are returning in case of error.
> 

Ok.

> @@ +315,3 @@
> +
> +                        if (read_bytes < 0) {
> +                            if (read_error) {
> 
> I think g_input_stream_read always fills the error in case of error.

Ok.

> @@ +332,3 @@
> +                            return FALSE;
> +                        } else if (read_bytes) {
> +                            g_byte_array_append (array,
> 
> Why are you caching the data instead of just calling ddjvu_stream_write() for
> every chunk read?

Must have confused something here about calling write once per stream. Sorry. Now I call it for every chunk, so I removed GByteArray, used chunk size of 1024 for buffer which is now alloced on stack.

> @@ +339,3 @@
> +                        }
> +                    }
> +                    ddjvu_stream_write (doc,
> 
> Do you really need to write all the data in one step?

Created attachment 216092 [details] [review]
Adds loading DJVU documents from memory, v3.

New patch. Without while/switch loop simplification for now.

(In reply to comment #21)
> > @@ +271,3 @@
> > +
> > +    if (!doc) {
> > +        djvu_handle_events (djvu_document, TRUE, &djvu_error);
> > 
> > Is this possible? what error can happen if we have just created the document
> > without giving a filename to decode?
> 
> I am just following one of general conventions in ddjvuapi.h:
> - all functions returning a pointer might return a null pointer.

Sure, what I meant was whether it's possible to receive an error message at this point. I don't think so, so you can just fill an error without waiting for an error message like we do in djvu_document_load().

> > @@ +297,3 @@
> > +    while ((msg = ddjvu_message_peek (ctx))) {
> > +        switch (msg->m_any.tag) {
> > +            case DDJVU_NEWSTREAM: {
> > 
> > I think this while + switch could be simplified by using 
> > 
> > djvu_wait_for_message (djvu_document, DDJVU_NEWSTREAM, &djvu_error);
> > 
> > First message should be a newstream, and first stream is always streamid = 0.
> > After reading the stream then you can do 
> > 
> > djvu_wait_for_message (djvu_document, DDJVU_DOCINFO, &djvu_error);
> > 
> > That would ignore all other streams (that we don't support yet) and wait for
> > docinfo message.
> 
> Hm, but shouldn't we call ddjvu_stream_close() for every stream, be it
> supported or not? I mean - newstream messages probably have to be handled as
> well.

Right.

> Also, the structure of this loop is just a copy of what was proposed in
> ddjvuapi.h.

Yes, but in the middle of this function it makes the function a bit complex in my opinion. I'm not sure it's possible to receive a DOCINFO message at that point, but if it's possible we could move the switch to a new function, something like this:

gboolean djvu_read_streams (DjvuDocument *djvu_document, GInputStream *stream, GError **error);

if something fails or DOCINFO message hasn't been received the function returns FALSE and fills the error. That would simplify the caller to do something like:

if (!djvu_read_streams (djvu_document, DDJVU_DOCINFO, &djvu_error)) {
        g_set_error_literal (error,
	                             EV_DOCUMENT_ERROR,
                                     EV_DOCUMENT_ERROR_INVALID,
                                     djvu_error->message);
        g_error_free (djvu_error);
        ddjvu_document_release (djvu_document->d_document);
        djvu_document->d_document = NULL;

        return FALSE;
}

Review of attachment 216092 [details] [review]:

::: backend/djvu/djvu-document.c
@@ +305,3 @@
+
+					do {
+#define CHUNKSIZE 1024

No need to define it here and undefine it later, just define it globally. I would call it something like READ_BUFFER_SIZE

g_file_load_contents() uses a block size of 8192, so we probably should copy that if just for consistency.

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME'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.gnome.org/GNOME/evince/issues/233.