Hi Carlos, Thank you for your valuable suggestions. I made the changes according to that. I attached a new patch with this. - I have some doubt when setting a 'Rendition' action, we have two options one is to set 'OP' other one is 'JS'. I am asking the user to pass the Javascript and set it as a stream here. - One more is i wrote a new function 'get_file_name_from_path' is it fine? what is the good place to add this function. Please give suggestions again. Thanks -- A Srinivas <div class="gmail_quote"> On Mon, Mar 21, 2011 at 12:30 AM, <<a href="mailto:poppler-request@lists.freedesktop.org">poppler-request@lists.freedesktop.org</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"> Send poppler mailing list submissions to <a href="mailto:poppler@lists.freedesktop.org">poppler@lists.freedesktop.org</a> To subscribe or unsubscribe via the World Wide Web, visit <a href="http://lists.freedesktop.org/mailman/listinfo/poppler" target="_blank">http://lists.freedesktop.org/mailman/listinfo/poppler</a> or, via email, send a message with subject or body 'help' to <a href="mailto:poppler-request@lists.freedesktop.org">poppler-request@lists.freedesktop.org</a> You can reach the person managing the list at <a href="mailto:poppler-owner@lists.freedesktop.org">poppler-owner@lists.freedesktop.org</a> When replying, please edit your Subject line so it is more specific than "Re: Contents of poppler digest..." Today's Topics: 1. Re: [PATCH] Simplify the GCC compiler flags/warnings (Albert Astals Cid) 2. Re: glib/poppler-annot.cc glib/poppler-annot.h poppler/Annot.cc poppler/Annot.h (Carlos Garcia Campos) ---------------------------------------------------------------------- Message: 1 Date: Sun, 20 Mar 2011 13:59:24 +0000 From: Albert Astals Cid <<a href="mailto:aacid@kde.org">aacid@kde.org</a>> Subject: Re: [poppler] [PATCH] Simplify the GCC compiler flags/warnings To: <a href="mailto:poppler@lists.freedesktop.org">poppler@lists.freedesktop.org</a> Message-ID: <<a href="mailto:201103201359.24744.aacid@kde.org">201103201359.24744.aacid@kde.org</a>> Content-Type: Text/Plain; charset="iso-8859-1" A Diumenge, 13 de mar? de 2011, Albert Astals Cid va escriure: > A Diumenge, 13 de mar? de 2011, Pino Toscano va escriure: > > Alle domenica 13 marzo 2011, Albert Astals Cid ha scritto: > > > A Dissabte, 12 de mar? de 2011, Pino Toscano va escriure: > > > > Alle sabato 12 marzo 2011, Hib Eris ha scritto: > > > > > Two more remarks: > > > > > > > > > > +if test "x$GCC" == xyes; then > > > > > + extra_cxxflags="-Wall -Wno-write-strings -Woverloaded-virtual > > > > > \ + -Wnon-virtual-dtor -Wcast-align > > > > > -fno-exceptions \ + -fno-check-new -fno-common" > > > > > + case "$enable_compile_warnings" in > > > > > + yes) > > > > > + extra_cxxflags="$extra_cxxflags \ > > > > > + -D_XOPEN_SOURCE=600 -D_BSD_SOURCE \ > > > > > + -W -Wno-long-long -Wundef -Wconversion > > > > > -Wpointer-arith \ + -Wwrite-strings > > > > > -Wformat-security \ > > > > > + -Wmissing-format-attribute" > > > > > + ;; > > > > > + esac > > > > > + CXXFLAGS="$extra_cxxflags $CXXFLAGS" > > > > > +fi > > > > > > > > > > > > > > > 1. In the 'yes' case, you end up with both -Wno-write-strings > > > > > and -Wwrite-strings, it is at least not obvious which one the > > > > > compiler will pick. > > > > > > > > The last of course. > > > > > > I disagree with that. It will result in people sending patches to > > > move all char * to const char* which adds nothing but noise to the > > > git history. > > > > -Write-strings is in the new "yes", which is the equivalent of the > > current "kde", so nothing changes for the default configuration. > > True, but the current "kde" is something noone uses, if you name it "enable > warnings" everyone will think it is our default "warnings" setting. Since it seems noone else cares i'd suggest you go my "simple" way with only one set of warnings, if we ever need more we can reintroduce the other "yes" warnings. Albert > > Albert > > > > I'd just scrap the "extra warnings" setting and by default enable the > > > ones we think make sense and that's it. > > > > That may be another idea -- other opinions? > > _______________________________________________ > poppler mailing list > <a href="mailto:poppler@lists.freedesktop.org">poppler@lists.freedesktop.org</a> > <a href="http://lists.freedesktop.org/mailman/listinfo/poppler" target="_blank">http://lists.freedesktop.org/mailman/listinfo/poppler</a> ------------------------------ Message: 2 Date: Sun, 20 Mar 2011 16:16:26 +0100 From: Carlos Garcia Campos <<a href="mailto:carlosgc@gnome.org">carlosgc@gnome.org</a>> Subject: Re: [poppler] glib/poppler-annot.cc glib/poppler-annot.h poppler/Annot.cc poppler/Annot.h To: aacid <<a href="mailto:aacid@kemper.freedesktop.org">aacid@kemper.freedesktop.org</a>> Cc: poppler <<a href="mailto:poppler@lists.freedesktop.org">poppler@lists.freedesktop.org</a>> Message-ID: <1300629799-sup-5028@charizard> Content-Type: text/plain; charset="utf8" Excerpts from aacid's message of jue mar 17 21:10:41 +0100 2011: > glib/poppler-annot.cc | 33 ++++ > glib/poppler-annot.h | 5 > poppler/Annot.cc | 377 ++++++++++++++++++++++++++++++++++++++++++++++++++ > poppler/Annot.h | 2 > 4 files changed, 417 insertions(+) > > New commits: > commit 66575c990f379871e4b796befc899de178332670 > Author: Srinivas Adicherla <<a href="mailto:srinivas.adicherla@gmail.com">srinivas.adicherla@gmail.com</a>> > Date: Thu Mar 17 20:14:05 2011 +0000 > > Patch for embedding videos in to the pdf > > diff --git a/glib/poppler-annot.cc b/glib/poppler-annot.cc > index 62a0879..c714288 100644 > --- a/glib/poppler-annot.cc > +++ b/glib/poppler-annot.cc > @@ -332,6 +332,39 @@ _poppler_annot_screen_new (Annot *annot) > return poppler_annot; > } I'm sorry I didn't find the time to review previous patches and I was on vacation when Albert asked for approval before commiting it. There are some problems with the patch, I'll comment inline. > +/** > + * poppler_annot_screen_new: > + * @doc: a #PopplerDocument > + * @rect: a #PopplerRectangle > + * @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 > + * @img_file: pass png/jpeg image file for poster > + * > + * 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 > + * > + * Since: 0.18 > + */ > +PopplerAnnot * > +poppler_annot_screen_new (PopplerDocument *doc, > + PopplerRectangle *rect, > + const char* video_file, > + const char* mimetype, > + const char* img_file) A screen annotation can be used to trigger any action, not only rendition actions. This should be just PopplerAnnot * poppler_annot_screen_new (PopplerDocument *doc, PopplerRectangle *rect); And another method to set the action poppler_annot_screen_set_action(), since the activation action is not required in a screen annotation. > +{ > + AnnotScreen *annot; > + PDFRectangle pdf_rect(rect->x1, rect->y1, > + rect->x2, rect->y2); > + > + annot = new AnnotScreen (doc->doc->getXRef(), &pdf_rect, doc->doc->getCatalog()); > + if (annot->setAction(video_file, mimetype, img_file)) > + return _poppler_annot_screen_new ((Annot*)annot); > + else > + return NULL; This function should never return NULL, since the action is optional there could be a screen annot without an action. > + > +GBool AnnotScreen::setAction(const char *video_file, const char *mimetype, const char *img_file) { There's a AnnotScreen::getAction() method that returns the LinkAction associated with the annotation, but this method doesn't update the current annotation action. Since the action might be of any type, I think this should be void AnnotScreen::setAction(LinkAction *action) if the annotation already had an action it will be replaced by the new one and the annot dictionary will be updated using Annot::update() > + if (!video_file) { > + error(-1, "Need to pass the video file"); > + return gFalse; > + } > + > + FILE *fs; > + unsigned int size = 0; > + if (!(fs = fopen(video_file, "rb"))) { > + error(-1, "Couldn't open video file '%s'", video_file); > + return gFalse; > + } > + fseek(fs, 0, SEEK_END); > + size = ftell(fs); > + > + // Extract the video name from the file uri > + const char *video_name = strrchr(video_file, '/'); > + if (video_name) { > + video_name++; > + } else { > + video_name = video_file; > + } > + > + Object obj1, obj2, obj3, obj4; > + > + annotObj.dictSet("T", obj1.initString(new GooString(video_name))); // title This should be another method AnnotScreen::setTitle() instead of always using the vodeo name. Since this is optional it's not a problem if the entry is not present unless the user explicitely calls setTitle() > + annotObj.dictSet("Contents", obj2.initString(new GooString(video_name))); // alternate text to be dispalyed for the > annotation The same here, we already have Annot::setContents(), use it to make sure the contents are correctly encoded and that the contents variable is correctly updated. > + annotObj.dictSet("F", obj3.initInt(4)); // No Zoom 4 is the bit position within the flag word, not the value itself. The spec doesn't say > + Object formXObj; > + formXObj.initDict(xref); > + > + formXObj.dictSet("Type", obj1.initName("XObject")); > + formXObj.dictSet("Subtype", obj2.initName("Form")); > + > + Object bbox; > + bbox.initArray(xref); > + bbox.arrayAdd(obj1.initReal(0.0000)); > + bbox.arrayAdd(obj2.initReal(0.0000)); > + bbox.arrayAdd(obj3.initReal(1.0000)); > + bbox.arrayAdd(obj4.initReal(1.0000)); > + formXObj.dictSet("BBox", &bbox); why is BBox 0, 0, 1, 1, shouldn't you use the annotation rectangle? something like bbox[0] = bbox[1] = 0; bbox[2] = rect->x2 - rect->x1; bbox[3] = rect->y2 - rect->y1; not that we already have Annot::createForm() that might help here too. > + Ref imgRef; > + Object imgXObj; > + imgXObj.initDict(xref); > + imgXObj.dictSet("Type", obj1.initName("XObject")); > + imgXObj.dictSet("Subtype", obj2.initName("Image")); > + > + if (img_file) { //show the poster, else do not show anything > + FILE *imgfp; > + if (!(imgfp = fopen(img_file, "rb"))) { > + error(-1, "Couldn't open file: %s", img_file); > + return gFalse; > + } > + unsigned char h[10]; > + fread(h, 1, 10, imgfp); > + fseek(imgfp, 0, SEEK_SET); > + > + MemStream *imgStream = NULL; > + // Load the stream from the png file > + if (h[0] == 0x89 && h[1] == 0x50 && h[2] == 0x4E && h[3] == 0x47) { > + imgStream = load_from_png (imgfp, &imgXObj); > + // Load the stream from the jpeg file > + } else if(h[0] == 0xFF && h[1] == 0xD8 && h[6] == 0x4A && h[7] == 0x46 && h[8] == 0x49 && h[9] == 0x46) { > + imgStream = load_from_jpeg (imgfp, &imgXObj); > + } else { > + error(-1, "Image format cannot be supported, only png/jpeg\n"); > + return gFalse; > + } > + > + if (!imgStream) { > + obj1.free(); > + obj2.free(); > + obj3.free(); > + obj4.free(); > + bbox.free(); > + imgXObj.free(); > + return gFalse; the AP entry is optional, you can have a screen annotation with a rendition action associated that doesn't have an appearance stream, so I don't think we should return here. > + } > + > + obj1.initStream(imgStream); > + imgRef = xref->addIndirectObject(&obj1); > + > + obj2.initDict(xref); // Image XObject > + obj2.dictSet("Im1", obj3.initRef(imgRef.num, imgRef.gen)); > + obj4.initDict(xref); > + obj4.dictSet("XObject", &obj2); > + > + formXObj.dictSet("Resources", &obj4); > + > + GooString *newString = new GooString(); > + newString->append("/Im1 Do"); > + formXObj.dictSet("Length", obj1.initInt(newString->getLength())); > + > + MemStream *fstream = new MemStream(copyString(newString->getCString()), 0, newString->getLength(), &formXObj); > + fstream->setNeedFree(gTrue); > + delete(newString); > + > + obj1.initStream(fstream); > + Ref appRef = xref->addIndirectObject(&obj1); > + > + obj2.initDict(xref); > + obj2.dictSet("N", obj3.initRef(appRef.num, appRef.gen)); > + obj2.dictSet("R", obj3.initRef(appRef.num, appRef.gen)); The spec says: "The AP entry refers to an appearance dictionary (see Table 168) whose normal appearance provides the visual appearance for a screen annotation that shall be used for printing and default display when a media clip is not being played. If AP is not present, the screen annotation shall not have a default visual appearance and shall not be printed." it talks about the normal appearance, I don't think we need to set the rollover appearance too. > + annotObj.dictSet("AP", &obj2); I think this should be moved to a generic method Annot::setAppearanceFromImage(const char *imageFile); or something like that. > + formXObj.initDict(xref); > + formXObj.dictSet("Type", obj3.initName("XObject")); > + formXObj.dictSet("Subtype", obj4.initName("Form")); > + > + bbox.initArray(xref); > + bbox.arrayAdd(obj1.initReal(0.0000)); > + bbox.arrayAdd(obj2.initReal(0.0000)); > + bbox.arrayAdd(obj3.initReal(1.0000)); > + bbox.arrayAdd(obj4.initReal(1.0000)); > + formXObj.dictSet("BBox", &bbox); Why 0, 0, 1, 1 here again? > + obj2.initDict(xref); > + obj2.dictSet("Im1", obj3.initRef(imgRef.num, imgRef.gen)); > + obj1.initDict(xref); > + obj1.dictSet("XObject", &obj2); > + formXObj.dictSet("Resources", &obj1); > + > + newString = new GooString(); > + newString->append("/Im1 Do"); > + formXObj.dictSet("Length", obj1.initInt(newString->getLength())); > + > + fstream = new MemStream(copyString(newString->getCString()), 0, newString->getLength(), &formXObj); > + fstream->setNeedFree(gTrue); > + delete(newString); > + > + obj1.initStream(fstream); > + appRef = xref->addIndirectObject(&obj1); > + > + obj2.initDict(xref); // MK dictionary > + obj2.dictSet("I", obj3.initRef(appRef.num, appRef.gen)); > + annotObj.dictSet("MK", &obj2); There's a AnnotScreen::getAppearCharacs() method that should refelct this change too > + } > + > + // Rendition Action to be add > + Object actionDict; > + actionDict.initDict(xref); > + actionDict.dictSet("Type", obj1.initName("Action")); > + actionDict.dictSet("S", obj2.initName("Rendition")); > + actionDict.dictSet("OP", obj3.initInt(0)); OP = 0 means: play the rendition specified by R, associating it with the annotation specified by AN, which makes AN a required field. AN should be an indirect reference to the screen annotation. I think we could add a new constructor to LinkRendition() that takes the required fields of a rendition action like we currenlty do with annotations. Then this method would simply take the action object and update the A entry with the indirect reference to the action. > + // Media Rendition > + Object MRendition; > + MRendition.initDict(xref); > + MRendition.dictSet("S", obj1.initName("MR")); > + MRendition.dictSet("N", obj2.initString(new GooString(video_name))); Same here, we could have a new constructor for MediaRendition that creates the new media rendition object. > + // Media Clip Dictionary > + Object MClipDict; > + MClipDict.initDict(xref); > + MClipDict.dictSet("S", obj1.initName("MCD")); > + MClipDict.dictSet("N", obj2.initString(new GooString(video_name))); > + > + if (mimetype) { > + MClipDict.dictSet("CT", obj3.initString(new GooString(mimetype))); > + } > + > + obj4.initString(new GooString("TEMPACCESS")); > + obj1.initDict(xref); > + obj1.dictSet("TF", &obj4); > + MClipDict.dictSet("P", &obj1); > + > + // File Specification Dictionary > + Object fsDict; > + fsDict.initDict(xref); > + fsDict.dictSet("Type", obj1.initName("Filespec")); > + fsDict.dictSet("F", obj2.initName((char*)video_name)); > + fsDict.dictSet("UF", obj3.initName((char*)video_name)); The UF entry should be encoded using PDFDocEncoding or UTF-16BE with a leading byte-order marker (as defined in 7.9.2.2, "Text String Type"). > + > + obj3.initDict(xref); // file stream eictionary > + obj3.dictSet("DL", obj1.initInt(size)); > + obj3.dictSet("Length", obj2.initInt(size)); > + > + FileStream *stream = new FileStream(fs, 0, 0, size, &obj3); > + obj1.initStream(stream); > + > + Ref newRef; > + newRef = xref->addIndirectObject(&obj1); > + obj2.initRef(newRef.num, newRef.gen); > + obj1.initDict(xref); > + obj1.dictSet("F", &obj2); > + fsDict.dictSet("EF", &obj1); This code could be useful for other annots, like FileAttachment annotations, so I think it could be moved to a protected method in Annot, something like void Annot::createFilespec(const char *filename); > + newRef = xref->addIndirectObject(&fsDict); > + obj1.initRef(newRef.num, newRef.gen); > + MClipDict.dictSet("D", &obj1); > + > + newRef = xref->addIndirectObject(&MClipDict); > + obj1.initRef(newRef.num, newRef.gen); > + MRendition.dictSet("C", &obj1); > + > + newRef = xref->addIndirectObject(&MRendition); > + obj1.initRef(newRef.num, newRef.gen); > + actionDict.dictSet("R", &obj1); > + > + newRef = xref->addIndirectObject(&actionDict); > + obj1.initRef(newRef.num, newRef.gen); > + annotObj.dictSet("A", &obj1); > + > + return gTrue; > +} > + Thank you very much for the patch, and sorry again for not reviewing it before. Regards, -- Carlos Garcia Campos PGP key: <a href="http://pgp.mit.edu:11371/pks/lookup?op=get&search=0x523E6462" target="_blank">http://pgp.mit.edu:11371/pks/lookup?op=get&search=0x523E6462</a> -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 198 bytes Desc: not available URL: <<a href="http://lists.freedesktop.org/archives/poppler/attachments/20110320/5c61136e/attachment-0001.pgp" target="_blank">http://lists.freedesktop.org/archives/poppler/attachments/20110320/5c61136e/attachment-0001.pgp</a>> ------------------------------ _______________________________________________ poppler mailing list <a href="mailto:poppler@lists.freedesktop.org">poppler@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/poppler" target="_blank">http://lists.freedesktop.org/mailman/listinfo/poppler</a> End of poppler Digest, Vol 73, Issue 27 *************************************** </blockquote></div>