Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add video annotation support #5450

Conversation

harshavardhana
Copy link

This work is still work in progress, doesn't seem to work for http://acroeng.adobe.com/Test_Files/classic_multimedia//VolvoS40V50-Full.pdf

It does seem like it worked in the beginning, but doesn't seem to work for me.

Rebasing change from pull request #4376 to get some reviews and pitfalls.

@harshavardhana harshavardhana changed the title Rebased from - https://github.com/pramodhkp/pdf.js/tree/videosupport Rebased from - pdf.js/tree/videosupport Oct 27, 2014
@timvandermeij
Copy link
Contributor

Before we can close #4376 and continue the work on this here, the author of this commit needs to be amended. See https://stackoverflow.com/questions/750172/change-the-author-of-a-commit-in-git/1320317#1320317 on how to do this. @pramodhkp needs to be stated as the author of this commit.

@harshavardhana harshavardhana force-pushed the pr_out_rebased_from_https_github_com_pramodhkp_pdf_js_tree_videosupport branch from a548890 to 2290bb4 Compare October 28, 2014 18:12
@harshavardhana
Copy link
Author

Author updated as @pramodhkp

@timvandermeij
Copy link
Contributor

Thank you. Fixes #2787.

@harshavardhana harshavardhana force-pushed the pr_out_rebased_from_https_github_com_pramodhkp_pdf_js_tree_videosupport branch from 2290bb4 to 00f5452 Compare October 30, 2014 19:01
@harshavardhana harshavardhana changed the title Rebased from - pdf.js/tree/videosupport Add video annotation support Oct 30, 2014
param2.value = 'mini';
element.appendChild(param2);
} else {
console.error('Cant play video');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use warn from src/shared/util.js instead of console.error.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@Rob--W
Copy link
Member

Rob--W commented Nov 1, 2014

Could we show a link to download the video when the browser cannot play the video? Then the user can download the file and play it with vlc if they wish. UI TBD.

@yurydelendik
Copy link
Contributor

The video sometimes can be found on the attachment tab

@harshavardhana harshavardhana force-pushed the pr_out_rebased_from_https_github_com_pramodhkp_pdf_js_tree_videosupport branch from 19ce390 to ba74b4f Compare November 2, 2014 10:59

function guessContentType(dict) {
var extToMime = {
'avi' : 'video/avi',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: only use space after the colon, and not before it. This line should read:

'avi': 'video/avi',

The same applies on the next two lines as well.

@harshavardhana harshavardhana force-pushed the pr_out_rebased_from_https_github_com_pramodhkp_pdf_js_tree_videosupport branch 2 times, most recently from 0e29ef9 to c5c5992 Compare November 2, 2014 20:42
@harshavardhana
Copy link
Author

Tested with this example - http://dante.ctan.org/tex-archive/macros/latex/contrib/movie15/doc/overlay-example.pdf

While the plugin and video playing still shows up - if i allow the popups it just crashes chrome on OSX. Will be testing on Linux.

@harshavardhana harshavardhana force-pushed the pr_out_rebased_from_https_github_com_pramodhkp_pdf_js_tree_videosupport branch from c5c5992 to 84ae57c Compare November 11, 2014 04:06
@harshavardhana harshavardhana force-pushed the pr_out_rebased_from_https_github_com_pramodhkp_pdf_js_tree_videosupport branch from 84ae57c to 03686e7 Compare November 20, 2014 07:33
@harshavardhana harshavardhana force-pushed the pr_out_rebased_from_https_github_com_pramodhkp_pdf_js_tree_videosupport branch from 03686e7 to 9961b72 Compare January 7, 2015 22:55
@fbender
Copy link

fbender commented Nov 16, 2015

@timvandermeij What's blocking merging this PR (except rebasing, ping @harshavardhana )?

@fbender fbender mentioned this pull request Nov 16, 2015
@timvandermeij
Copy link
Contributor

Nothing big I think. It needs a rebase and testing (unit tests especially).

@harshavardhana harshavardhana force-pushed the pr_out_rebased_from_https_github_com_pramodhkp_pdf_js_tree_videosupport branch 2 times, most recently from 9667462 to 173f9a6 Compare November 26, 2015 08:28
@harshavardhana
Copy link
Author

Nothing big I think. It needs a rebase and testing (unit tests especially).

Can you tell me how would i go about adding those unit tests?

@humphd
Copy link

humphd commented Dec 8, 2015

I was trying your branch, but what's on github doesn't seem to work with that example pdf--the worker dying on

var annotationFactory = new AnnotationFactory();
with AnnotationFactory missing. Do you have something locally that fixes this?

}
}
};
var SUPPORTED_TYPES = ['Link', 'Text', 'Widget', 'Screen', 'Movie'];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is incorrect. The AnnotationFactory should not be removed (causing the effect that the user above describes), but rather it should be adapted to add a case for Screen and Movie.

@humphd
Copy link

humphd commented Dec 11, 2015

I played with this some more, and with the Annotation ctor restored in this PR, I can see VideoAnnotations being parsed correctly; however, the getHtmlElementForVideoAnnotation function in annotation_helper.js that should create the video element and usable UI, is never called. I don't know the pdf.js internals enough to fix this, but I assume that the VideoAnnotation needs to somehow indicate that it has backing HTML and viewable content, such that the stuff in pdfPage.getAnnotations() should create and render the element. Looking at how the LinkAnnotation (appears) to do it via data.hasHtml = true;, I wondered if this would be enough, but it isn't; I suspect something has to get parsed out of the annotation's data that isn't currently being done. Any tips?

@timvandermeij
Copy link
Contributor

data.hasHtml = true; should render the annotation and it is indeed absent in this PR, which should be changed. I have added some review comments above which might also contribute to the fact that the annotation is not rendered properly.

@humphd
Copy link

humphd commented Dec 12, 2015

I've made all the changes discussed above (the s/InteractiveAnnotation/Annotation/ stuff was already done), still no dice: humphd@635c061. @harshavardhana, I tried to send this to you as a PR, but github is being stupid and doesn't list you in the list of possible forks for pdf.js for some reason, so won't let me. @timvandermeij, what am I missing?

@harshavardhana harshavardhana force-pushed the pr_out_rebased_from_https_github_com_pramodhkp_pdf_js_tree_videosupport branch from 4b52236 to 635c061 Compare December 13, 2015 09:23
@harshavardhana
Copy link
Author

I've made all the changes discussed above (the s/InteractiveAnnotation/Annotation/ stuff was already done), still no dice: humphd/pdf.js@635c061. @harshavardhana, I tried to send this to you as a PR, but github is being stupid and doesn't list you in the list of possible forks for pdf.js for some reason, so won't let me. @timvandermeij, what am I missing?

I pulled in your changes and pushed here. Thank you

@@ -14,8 +14,9 @@
*/
/* globals PDFJS, Util, isDict, isName, stringToPDFString, warn, Dict, Stream,
stringToBytes, Promise, isArray, ObjectLoader, OperatorList,
isValidUrl, OPS, AnnotationType, stringToUTF8String,
AnnotationBorderStyleType, ColorSpace, AnnotationFlag, isInt */
isValidUrl, OPS, createPromiseCapability, AnnotationType,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes for these three lines should also be reverted.

@timvandermeij
Copy link
Contributor

/botio-linux preview

@pdfjsbot
Copy link

From: Bot.io (Linux)


Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://107.21.233.14:8877/af3f05d4b8ed724/output.txt

@@ -45,7 +45,8 @@ var ImageKind = {
var AnnotationType = {
WIDGET: 1,
TEXT: 2,
LINK: 3
LINK: 3,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks to PR #6757 the changes in this file are no longer necessary as types MOVIE and SCREEN are now already available when you rebase this patch.

@fbender
Copy link

fbender commented May 20, 2016

@harshavardhana Are you still working on this? May you please address the review comments and rebase the branch?

@timvandermeij
Copy link
Contributor

timvandermeij commented Sep 11, 2016

Closing as incomplete for the following reasons:

  • This hasn't been worked on for a long time.
  • The patch currently does not work for rendering video elements.
  • There are no tests.
  • The annotation code has been rewritten, so this needs to be fit into the new framework and therefore this patch is currently not in a mergeable state.

However, we keep the associated issue open and link to this PR from there so that others may use this work as inspiration for a new PR.

@harshavardhana harshavardhana deleted the pr_out_rebased_from_https_github_com_pramodhkp_pdf_js_tree_videosupport branch September 11, 2016 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants