Bug 40561

Summary: [PATCH] support for LinkMovie object in Qt4 frontend
Product: poppler Reporter: edward.hades
Component: qt4 frontendAssignee: poppler-bugs <poppler-bugs>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: medium    
Version: unspecified   
Hardware: All   
OS: All   
Whiteboard:
i915 platform: i915 features:
Attachments: partial support for Movie actions
updated patch as per comments
updated patch as per comments
Tex example, slightly modified to actually compile with pdflatex
The pdf that results
Updated patch for implementing support for LinkMovie
Integrated review
Integrated review comments from Albert
Next interation of integrating review comments

Description edward.hades 2011-09-01 10:21:20 UTC
This patch adds support for LinkMovie object (which is required to auto-start movies in PDF files).

Alternatively, if you prefer using Git directly:

The following changes since commit 91fafce028ca6620c0eb22e370fb4c6fd3404e3c:

  cairo: align strokes when Stroke Adjust is true and line width <= 1 (2011-08-28 13:09:07 +0200)

are available in the git repository at:
  git://git.hades.name/poppler.git movie

Edward Hades (1):
      qt4: partial support for Movie actions

 qt4/src/poppler-link.cc |   30 +++++++++++++++++++++---------
 qt4/src/poppler-link.h  |   32 ++++++++++++++++++++++++++------
 qt4/src/poppler-page.cc |   36 +++++++++++++++++++++++++++---------
 3 files changed, 74 insertions(+), 24 deletions(-)
Comment 1 edward.hades 2011-09-01 10:22:36 UTC
Created attachment 50825 [details] [review]
partial support for Movie actions
Comment 2 Albert Astals Cid 2011-09-01 13:56:52 UTC
Some quick comments:
 * Please try to keep spacing consistant with the rest of the file, at least the ifs are not correctly spaced
 * Please give all the parameters names
 * You pass r and g and then never use them, why?
 * All the data should be in the private, so please remove the protected OperationType member and put it in the private

Thanks for the patch :-)
Comment 3 edward.hades 2011-09-02 00:48:13 UTC
(In reply to comment #2)
> Some quick comments:
>  * Please try to keep spacing consistant with the rest of the file, at least
> the ifs are not correctly spaced

Have you noticed, you have at least two different styles across files? :)

>  * You pass r and g and then never use them, why?

An action can refer to a movie annotation either by title or by ref-gen pair. I've done the title, but I've skipped the ref-gen comparison for now, since MovieAnnotation does not store them (or at least I haven't found them). That's why I called the support "partial".
Comment 4 edward.hades 2011-09-02 00:48:46 UTC
Created attachment 50836 [details] [review]
updated patch as per comments
Comment 5 Albert Astals Cid 2011-09-02 01:27:53 UTC
OperationType type; is still a member of LinkMovie instead of LinkMoviePrivate (reading my comment i see that it was probably confusing in what i meant)
Comment 6 edward.hades 2011-09-02 01:35:45 UTC
Created attachment 50838 [details] [review]
updated patch as per comments
Comment 7 oliver.sander 2011-09-22 03:29:56 UTC
I tried the patch with your patched okular and the test file given in the first comments of this okular bug https://bugs.kde.org/show_bug.cgi?id=136574.

Unfortunately, for me the movie on the second slide does not start automatically, even though it should.

I placed a few printfs in your code to see what's going on.  It appears that the new isTarget method you added to poppler-link.cc is never called for the test file.
Comment 8 edward.hades 2011-09-22 08:14:25 UTC
That's very unfortunate. Can you debug it further?

If not, could you please provide the stderr output of okular?

If yes, the invokation of isTarget() is near generator_pdf.cpp:256

http://git.hades.name/cgit.cgi/okular.git/tree/generators/poppler/generator_pdf.cpp?h=movie&id=dbee5267271f2a634652e19827f2ba25e4345c98#n256

Could you check why is it not being invoked?

Could it by any chance be because of some kind of okular not linking to new okularGenerator_poppler.so ?
Comment 9 oliver.sander 2011-09-23 09:51:08 UTC
Created attachment 51551 [details]
Tex example, slightly modified to actually compile with pdflatex
Comment 10 oliver.sander 2011-09-23 09:51:52 UTC
Created attachment 51552 [details]
The pdf that results
Comment 11 oliver.sander 2011-09-23 09:54:21 UTC
I tested some more, and here is what I found.  The part about isTarget not being called was actually bogus.  I hadn't known about the kbuildsycoca4 call you have to make.

Now it does indeed work for the test file mentioned above.  Thanks!  However, it only works in presentation mode, and not in regular mode (but maybe that's intentional?).  Also, it does not work if you first go to a slide with an autostarting movie on it, and only then switch to presentation mode.  You need to be in presentation mode and then switch to the page with the movie.

Then, I wanted to try this with a pdf created by myself, using LaTeX and beamer as in the above example.  First I wanted to simply recompile the given example.  Strangely enough, that one cannot compile at all, because it references an eps file, which a) is not provided and b) pdflatex won't take eps files anyways.  So I patched that tex file a little bit, recompiled, and the movie does not autostart. 

Unfortunately I don't have a Windows machine near me to test whether at least there the movie autostarts.  I attached my patched tex file instead.
Comment 12 edward.hades 2011-09-23 10:23:34 UTC
(In reply to comment #11)

Thanks for testing!

The autostart indeed works only in presentation mode. This is because in normal mode Okular does not activate Actions associated with page opening. I am not sure if this is intended by Okular developers or a bug (which I haven't yet investigated), but in any case it's not a Poppler issue.

Regarding your PDF file, I am not sure where is the error in .tex file, but the resulting PDF does not contain the necessary code to autostart video (the movie action "46 0" is not referenced in page's actions). I am sure (although haven't checked) that it won't work in Windows also.
Comment 13 oliver.sander 2011-09-24 09:39:58 UTC
> The autostart indeed works only in presentation mode. This is because in normal
> mode Okular does not activate Actions associated with page opening. I am not
> sure if this is intended by Okular developers or a bug (which I haven't yet
> investigated), but in any case it's not a Poppler issue.

I am not sure what behavior to expect here, either.  I guess that is on Albert to decide.

> 
> Regarding your PDF file, I am not sure where is the error in .tex file, but the
> resulting PDF does not contain the necessary code to autostart video (the movie
> action "46 0" is not referenced in page's actions). I am sure (although haven't
> checked) that it won't work in Windows also.

I was afraid you were going to say this.  The tex-file requests autostarting of the movie in the way described by the 'beamer' latex package manual (by simply adding the 'autostart' option).  If this does not lead to a correct pdf does this mean it is a beamer bug?  And how did the original poster generate his demo pdf? I have no clue.

What about the issue that a movie will not autostart if you switch to presentation mode will showing the corresponding page?  I guess that could be annoying if you have an autostarting movie on your first slide.

Cheers,
Oliver
Comment 14 Albert Astals Cid 2011-09-25 17:51:03 UTC
(In reply to comment #13)
> I am not sure what behavior to expect here, either.  I guess that is on Albert
> to decide.

Personally i find it a bit agressive to have a video autostart if i am just browsing it (i.e. not in presentation mode)

> I was afraid you were going to say this.  The tex-file requests autostarting of
> the movie in the way described by the 'beamer' latex package manual (by simply
> adding the 'autostart' option).  If this does not lead to a correct pdf does
> this mean it is a beamer bug?  
Yes

> What about the issue that a movie will not autostart if you switch to
> presentation mode will showing the corresponding page?  

I'd probably call this a bug, but don't have a strong opinion
Comment 15 oliver.sander 2011-10-04 02:56:01 UTC
> Personally i find it a bit agressive to have a video autostart if i am just
> browsing it (i.e. not in presentation mode)

I tend to see this the same way.
 
> > What about the issue that a movie will not autostart if you switch to
> > presentation mode will showing the corresponding page?  
> 
> I'd probably call this a bug, but don't have a strong opinion

Even with this 'bug' the patch is a definite improvement.  Can we have it in master, please?
Comment 16 Albert Astals Cid 2011-10-04 03:13:00 UTC
I've already spoken with edward and he knows of that the patch needs to be improved before i commit it.
Comment 17 oliver.sander 2011-10-04 04:36:52 UTC
okay, thanks guys!
Comment 18 Albert Astals Cid 2012-02-14 14:56:28 UTC
Edward did you have time to improve the suggestions we talked about back in 2011-10-04 as said in comment #16 ?
Comment 19 Tobias Koenig 2012-03-06 04:48:17 UTC
Created attachment 58057 [details] [review]
Updated patch for implementing support for LinkMovie

The LinkMovie takes the title and the reference (new class) of the associated annotation now and provides static convenience methods to check whether a LinkMovie object or title/reference pair matches a MovieAnnotation.
Comment 20 Albert Astals Cid 2012-03-06 13:15:49 UTC
There's broken indentation in 
QString LinkMovie::annotationTitle() const
and
ObjectReference LinkMovie::annotationReference() const

bool LinkMovie::isReferencedAnnotation( const MovieAnnotation *annotation, const LinkMovie *link )
should not be static and use "this" instead of link

Why would you ever need
bool LinkMovie::isReferencedAnnotation( const MovieAnnotation *annotation, const QString &annotationTitle, const ObjectReference &annotationReference ) ?

Why someone would want to know the 
ObjectReference annotationReference() const; ?
Comment 21 Tobias Koenig 2012-03-07 06:57:25 UTC
Created attachment 58118 [details] [review]
Integrated review

This patch removes the isReferencedAnnotation() method that took title and reference as parameter. The intent was to not keep the Poppler::LinkMovie object around in Okular, but that's of course a design problem in Okular and shouldn't be reflected in Poppler API.
So LinkMovie provides a const member method now and hides the title/reference from public API.
Comment 22 Albert Astals Cid 2012-03-07 11:19:02 UTC
You should pass the other values to the LinkMoviePrivate constructor, this way you avoid the akward thing of having LinkMoviePrivate initialize operation to LinkMovie::Play and then do d->operation = operation;

Also i think it'd be a nice idea if you don't expose
void setPdfObjectReference(const ObjectReference &reference);
since i don't see a reason a user would use it, and simply go behind the scenes and update d->pdfObjectReference

The since should be 0.20, not 0.8 and are missing in the new functions

And last i remember we were trying to use QSharedDataPointers in stuff like ObjectReferencePrivate * const d; but i'll accept if you say "nah" on it.
Comment 23 Tobias Koenig 2012-03-08 03:03:26 UTC
Created attachment 58161 [details] [review]
Integrated review comments from Albert
Comment 24 Albert Astals Cid 2012-03-08 09:47:57 UTC
Since the objectreference is never passed anymore to the user, i think it'd make sense to turn the .h into a _p.h and not install it. What do you think?
Comment 25 Tobias Koenig 2012-03-09 11:23:40 UTC
(In reply to comment #24)
Hej Albert,

> Since the objectreference is never passed anymore to the user, i think it'd
> make sense to turn the .h into a _p.h and not install it. What do you think?
It is still passed to the LinkMovie ctor. Should this be made private as well?
Comment 26 Albert Astals Cid 2012-03-09 11:48:26 UTC
It is ok, it is a const & so just do a forward declare and it will work. Maybe document the constrcutor is private so people don't get crazy trying to create one :D
Comment 27 Tobias Koenig 2012-03-10 03:24:43 UTC
Created attachment 58264 [details] [review]
Next interation of integrating review comments

Make poppler-objectreference.h a private header now and document the LinkMovie ctor as being private.
Comment 28 Albert Astals Cid 2012-03-11 15:23:40 UTC
I've commited your patch with a minor change in which two invalid objectreferences are equal, having

ObjectReference a;
ObjectReference b;
bool c = a == b;

with c being false was too weird

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.