[poppler] Patch for embedding videos in to the pdf
Pino Toscano
pino at kde.org
Wed Mar 30 02:50:14 PDT 2011
Hi,
Alle mercoledì 30 marzo 2011, srinivas adicherla ha scritto:
> I made those changes suggested by you. Please find the latest
> patch attached with this mail.
> Thanks for all your suggestions.
Please note that this single patch is collecting now multiple additions
which are not strictly related to your final goal, so would be nice if
you could split it and send them separately as new emails (without
replying to this email, but each in a new mailing list thread);
reviewing a single blob isn't easy.
At least you have separate patches for:
- new additions in FileSpec (core only)
- the ref/getRef its for Link (core only)
- the possibility to set the title of an annotation, which looks
actually bogus (see comments below)
- AnnotScreen::setAction (core only)
- the new stuff for LinkRendition
- the additions in MediaRendition
and then whatever is left excluding those.
As a general note on the patch: there are lots of stray tabs and spaces
in the code you added; please go trough your code and clean them.
> Object *getAppearance(Object *obj) { return appearance.fetch(xref,
> obj);
> } -
> + GBool setAppearanceFromFile(const char *imgFile);
style: please leave the empty line
> + Ref *fileref;
> +
> + // File Specification Dictionary
> + if (stream)
> + fileref = createFilespecFromStream(xrefA, stream, filename);
> + else
> + fileref = createFilespec (xrefA, filename);
this way of using Ref is simply bad, will explain below
> +const char*
> +getFileNameFromPath (const char *filepath) {
style: return type and the rest go in the same line
> + const char *filename = strrchr(filepath, '/');
> + if (filename)
> + filename++;
> + else
> + filename = filepath;
theorically this should also take windows into account, but I guess we
need help form some windows porter...
> +static Ref*
> +getFileSpecObject(XRef *xrefA, Stream *stream, const char* filename)
same style as above
> + fileSpec.dictSet("F", obj1.initString(new
> GooString((char*)filename)));
why do you cast away the constness?
> + GooString *videoname = new GooString(filename);
there's no such "video" here, please give it a more meaningful name;
also, please allocate it on stack, instead of deleting it manually three
lnes below
> + char *filenameUTF16 = pdfDocEncodingToUTF16(videoname, &length);
somehow, there's a bit of memory waste here, first construct a GooString
and then converting it to utf-16... I guess we can add a
pdfDocEncodingToUTF16 overload which takes a const char* + its length
> + fileSpec.dictSet("UF", obj1.initString(new
> GooString(filenameUTF16, length)));
> +
> + free(filenameUTF16);
pdfDocEncodingToUTF16 returns a new char[], which you have to delete [],
not free() (as also Carlos said)
> + obj1.initStream(stream);
> +
> + Ref newRef = xrefA->addIndirectObject(&obj1);
> + obj2.initRef(newRef.num, newRef.gen);
> + obj1.initDict(xrefA);
> + obj1.dictSet("F", &obj2);
> + fileSpec.dictSet("EF", &obj1);
> +
> + Ref ref = xrefA->addIndirectObject(&fileSpec);
> + return &ref;
you're returning the address of something allocated on stack: this will
crash at the first occasion it can; do not ever do something like that.
just return the Ref.
> + error(-1, "Couldn't open video file '%s'", filepath);
> [...]
> + // Extract the video name from the file uri
again, no video here
> + char *filename = (char*) getFileNameFromPath(filepath);
why do you cast away the constness?
> -
> +Ref* createFilespecFromStream (XRef *xref, Stream *stream, const
> char* filename);
style: please leave the empty line
> Index: poppler-0.16.2/poppler/Link.h
> [...]
> #include "Object.h"
> +#include "Annot.h"
unuseful include here, just forward-declare Annot;
> + // Get the reference object of the corresponding action
> + virtual Ref getRef() = 0;
what's the use of making this pure virtual, if in (almost, except
LinkRendition) every LinkAction subclass you add a Ref class member and
return it?
I'd just make it virtual, with a default implementation returning a
Ref() object
> +void Annot::setTitle(GooString *title) {
> + if (title) {
> + title = new GooString(title);
> + if (!title->hasUnicodeMarker()) {
> + title->insert(0, 0xff);
> + title->insert(0, 0xfe);
> + }
> + }
> + else
> + title = new GooString();
this is one of the occasions I miss -Wshadow in the CXXFLAGS...
you are reusing the parameter name for the new object, which means you
won't be able to access to the parameter.
> + Object obj1;
> + obj1.initString(title->copy());
> + update ("T", &obj1);
> +}
T is a key of Markup Annotation dictionaries, not of any Annotation;
which means the AnnotMarkup::setLabel() is what you have already
> +static MemStream* load_from_png (FILE *f, Object *imgXObj) {
I'm thinking that somehow we should have some ImgWriter or ImgLoader
structure (similar to ImgWriter), instead of putting everything into
Annots; right now I'm not sure how to do it best, this is worth a
discussion
> +GBool Annot::setAppearanceFromFile(const char *img_file) {
> + assert(img_file != NULL); // checking for safe
err no, design-by-contract like that is a bit over, just return cleanly
if the img_file is NULL
> + unsigned char h[10];
> + fread(h, 1, 10, imgfp);
> + fseek(imgfp, 0, SEEK_SET);
you are missing any kind of check on the return values of the two, for
example on i/o errors or simply because the passed file could be even
shorter than 10 bytes
> + double bbox[4];
> + bbox[0] = bbox[1] = 0;
> + bbox[2] = bbox[3] = 1;
quoting Carlos:
> Why is bbox 0, 0, 1, 1? Shouldn't it be the annot rectangle?
> +void AnnotScreen::setAction(LinkAction *action) {
> + Object obj;
> + Ref ref = action->getRef();
> + obj.initRef(ref.num, ref.gen);
> + update("A", &obj);
> +}
this is completely forgetting to delete the old action (the class
member) and set the new action
also, I guess it should check for a valid ref before do the update, or
discard the new action altogether
> +static gboolean
> +poppler_media_get_stream_from_file (XRef *xrefA, const char *file,
> Stream **media_stream)
looks like an internal function, so make its name start with an
underscore
> +{
> + FILE *fp;
> + if (!(fp = fopen(file, "rb")))
> + return FALSE;
> +
> + unsigned int size = 0;
> + fseek(fp, 0, SEEK_END);
> + size = ftell(fp);
as above: you are missing any check on the i/o operations, which can
fail
> + Object obj1, obj2;
> + obj1.initDict(xrefA);
> + obj1.dictSet("Length", obj2.initInt(size));
> + *media_stream = new FileStream(fp, 0, 0, size, &obj1);
> +}
return TRUE is missing
> /**
> +* poppler_media_from_filei:
> +* @doc: a #PopplerDocument
> +* @video_file: a video file to be embedded
> +* @mimetype: mimetype of the video
> +* @isEmbed: if TRUE embed the video by passing stream, else embed it
> from file
> +*
> +* Return value: a #PopplerMedia
> +*
> +* Since: 0.14
for sure at least it is 0.18
> +*/
> +PopplerMedia *
> +poppler_media_from_file (PopplerDocument *doc, const char
> *video_file, const char *mimetype, gboolean isEmbed)
glib style: isEmbed -> is_embedded
> +{
> + PopplerMedia *media;
> +
> + g_assert (video_file != NULL);
I guess this should be a g_return_val_if_fail or something like that,
and needs a check for doc as well
> +PopplerAction *poppler_action_rendition_new (PopplerAnnot *annot,
> + PopplerMedia *media,
> + const char* video_title,
> + guint operation);
> +PopplerAction
> *poppler_action_rendition_new_for_javascript (PopplerAnnot *annot,
> + const
> char* js);
there's a bit inconsistent use of tabs and spaces in the indentation
> +/**
> + * poppler_annot_screen_new:
> + * @doc: a #PopplerDocument
> + * @rect: a #PopplerRectangle
> + *
> + * Creates a new Screen annotation that will be
> + * located on @rect when added to a page. See
> + * poppler_page_add_annot()
> + *
> + * Return value: A newly created #PopplerAnnotScreen annotation
> + *
> + */
missing the Since: ...
> +PopplerAnnot *
> +poppler_annot_screen_new (PopplerDocument *doc,
> + PopplerRectangle *rect)
> +{
> + Annot *annot;
> + PDFRectangle pdf_rect(rect->x1, rect->y1,
> + rect->x2, rect->y2);
style: single line here
missing a g_return_val_if_fail checking for doc and rect
> /**
> + * poppler_annot_set_title:
> + * @poppler_annot: a #PopplerAnnot
> + * @title: a text string containing the new contents
> + *
> + * Sets the title of @poppler_annot.
> + *
> + **/
> +void
> +poppler_annot_set_title (PopplerAnnot *poppler_annot,
> + const gchar *title)
following what I said above, this isn't needed, you have
poppler_annot_markup_set_label already
> +/**
> + * poppler_annot_set_appearance_from_file:
> + * @poppler_annot: a #PopplerAnnot
> + * @img_file: an image file to set as appearance in annots rectangle
> area + *
> + * Sets the appearance of @poppler_annot
> + *
> + **/
again, missing Since: ...
> +void
> +poppler_annot_set_appearance_from_file (PopplerAnnot *poppler_annot,
> + const gchar *img_file)
> +{
> +
> + poppler_annot->annot->setAppearanceFromFile(img_file);
again, missing a g_return_val_if_fail checking for poppler_annot and
img_file
> Index: poppler-0.16.2/glib/poppler-private.h
> ===================================================================
> --- poppler-0.16.2/glib/poppler-private.h (revision 12)
> +++ poppler-0.16.2/glib/poppler-private.h (working copy)
> @@ -10,6 +10,7 @@
> #include <Movie.h>
> #include <Rendition.h>
> #include <Form.h>
> +#include <FileSpec.h>
you don't need this here, but only in the .cc files that need it
> Index: poppler-0.16.2/glib/poppler-action.cc
> ===================================================================
> --- poppler-0.16.2/glib/poppler-action.cc (revision 12)
> +++ poppler-0.16.2/glib/poppler-action.cc (working copy)
> @@ -665,3 +665,79 @@
> {
> return dest_new_goto (document, link_dest);
> }
> +
> +/*
> + * poppler_action_create_rendition_new:
> + * @poppler_annot: a #PopplerAnnot
> + * @video_file: pass the path of the video to be embed
> + * @mimetype: pass the mimetype of the video to put in the Media
> Clip Dictionary + * @video_title: Title of the video to set
> +
> + * Set the action for the screen annotation specified @poppler_annot
> + *
> + */
again, missing Since: ...
> + if (media->stream)
> + rendition = new MediaRendition (xref, media->stream,
> media->filename, media->mime_type);
> + else
> + rendition = new MediaRendition (xref, NULL, media->filename,
> media->mime_type);
if media->stream is NULL, why the distinction?
> +/*
> + * poppler_action_create_rendition_new:
> + * @poppler_annot: a #PopplerAnnot
> + * @js: Javascript to execute
> +
> + * Set the action for the screen annotation specified @poppler_annot
> + *
> + */
again, missing Since: ...
> +PopplerAction*
> +poppler_action_rendition_new_for_javascript (PopplerAnnot
> *poppler_annot,
> + const char* js)
shouldn't this be a gchar* or similar?
> +{
> + g_return_val_if_fail (js != NULL, NULL);
> +
> + PopplerAction *action;
> + action = g_slice_new0 (PopplerAction);
> +
> + XRef *xref = poppler_annot->annot->getXRef();
> + LinkRendition *rendition;
> + Object jsObj, obj1, obj2;
> + obj1.initDict(xref);
> + obj1.dictSet("Length", obj2.initInt(strlen(js)));
> + MemStream *jsstream = new MemStream(copyString((char*)js), 0,
> strlen(js), &obj1);
why do you init the dictionary for the MemStream with the XRef?
--
Pino Toscano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 190 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/poppler/attachments/20110330/1f9d60fa/attachment.pgp>
More information about the poppler
mailing list