From 79a0d34adf92b1cc8900d84bc2f7d419b6c35ce1 Mon Sep 17 00:00:00 2001 From: Noah Levitt Date: Mon, 29 Apr 2019 15:29:16 -0700 Subject: [PATCH 1/9] ExtractorYoutubeDL --- .../modules/extractor/ExtractorYoutubeDL.java | 297 ++++++++++++++++++ .../crawler/io/UriProcessingFormatter.java | 1 - 2 files changed, 297 insertions(+), 1 deletion(-) create mode 100644 contrib/src/main/java/org/archive/modules/extractor/ExtractorYoutubeDL.java diff --git a/contrib/src/main/java/org/archive/modules/extractor/ExtractorYoutubeDL.java b/contrib/src/main/java/org/archive/modules/extractor/ExtractorYoutubeDL.java new file mode 100644 index 00000000..16a2ce87 --- /dev/null +++ b/contrib/src/main/java/org/archive/modules/extractor/ExtractorYoutubeDL.java @@ -0,0 +1,297 @@ +package org.archive.modules.extractor; + +import java.io.IOException; +import java.io.InputStreamReader; +import java.io.Reader; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.logging.Level; +import java.util.logging.Logger; + +import org.apache.commons.httpclient.URIException; +import org.archive.crawler.reporting.CrawlerLoggerModule; +import org.archive.modules.CoreAttributeConstants; +import org.archive.modules.CrawlURI; +import org.archive.net.UURI; +import org.archive.net.UURIFactory; +import org.archive.util.ArchiveUtils; +import org.archive.util.MimetypeUtils; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.context.Lifecycle; + +import com.google.gson.JsonObject; +import com.google.gson.JsonParseException; +import com.google.gson.JsonStreamParser; + +public class ExtractorYoutubeDL extends Extractor implements Lifecycle { + + private static Logger logger = + Logger.getLogger(ExtractorYoutubeDL.class.getName()); + + protected transient Logger ydlLogger = null; + + protected CrawlerLoggerModule crawlerLoggerModule; + public CrawlerLoggerModule getCrawlerLoggerModule() { + return this.crawlerLoggerModule; + } + @Autowired + public void setCrawlerLoggerModule(CrawlerLoggerModule crawlerLoggerModule) { + this.crawlerLoggerModule = crawlerLoggerModule; + } + + @Override + public void start() { + if (!isRunning) { + // loggerModule.start() creates the log directory, and it might be + // possible for this module to start before loggerModule, so we need + // to run this here to prevent an exception + getCrawlerLoggerModule().start(); + + ydlLogger = getCrawlerLoggerModule().setupSimpleLog(getBeanName()); + } + super.start(); + } + + protected String readToEnd(Reader r) throws IOException { + StringBuilder buf = new StringBuilder(); + char[] rbuf = new char[4096]; + while (true) { + int n = r.read(rbuf); + if (n < 0) { + return buf.toString(); + } + buf.append(rbuf, 0, n); + } + } + + /** + * - If {@code uri} is annotated "youtube-dl" and is a 3xx (redirect), + * find the redirect among the outlinks and add the "youtube-dl" + * annotation to it as well, and also make a note of the containing page + * inside the CrawlURI. {@link ExtractorHTTP} needs to have have run + * already. + * + * - If {@code uri} is annotated "youtube-dl" and is an actual video + * download, log a line to ExtractorYoutubeDL.log + * + * - If {@link #shouldExtract(CrawlURI)}, do youtube-dl extraction. + */ + @Override + protected void extract(CrawlURI uri) { + String ydlAnnotation = findYdlAnnotation(uri); + if (ydlAnnotation != null) { + if (uri.getFetchStatus() >= 300 && uri.getFetchStatus() < 400) { + doRedirectInheritance(uri, ydlAnnotation); + } else { + logCapturedVideo(uri, ydlAnnotation); + } + } else { + List ydlJsons = runYoutubeDL(uri); + for (JsonObject json: ydlJsons) { + if (json.get("url") != null) { + String videoUrl = json.get("url").getAsString(); + addVideoOutlink(uri, json, videoUrl); + } + } + } + } + + private void addVideoOutlink(CrawlURI uri, JsonObject json, + String videoUrl) { + try { + UURI dest = UURIFactory.getInstance(uri.getUURI(), videoUrl); + CrawlURI link = uri.createCrawlURI(dest, LinkContext.EMBED_MISC, + Hop.EMBED); + String annotation = "youtube-dl:1/1"; + if (!json.get("playlist_index").isJsonNull()) { + annotation = "youtube-dl:" + json.get("playlist_index") + "/" + + json.get("n_entries"); + } + link.getAnnotations().add(annotation); + uri.getOutLinks().add(link); + } catch (URIException e) { + logUriError(e, uri.getUURI(), videoUrl); + } + } + + protected String findYdlAnnotation(CrawlURI uri) { + for (String annotation: uri.getAnnotations()) { + if (annotation.startsWith("youtube-dl:")) { + return annotation; + } + } + return null; + } + + protected void logCapturedVideo(CrawlURI uri, String ydlAnnotation) { + // "length" logic copied from UriProcessingFormatter + String length = "-"; + if (uri.isHttpTransaction()) { + if(uri.getContentLength() >= 0) { + length = Long.toString(uri.getContentLength()); + } else if (uri.getContentSize() > 0) { + length = Long.toString(uri.getContentSize()); + } + } else { + if (uri.getContentSize() > 0) { + length = Long.toString(uri.getContentSize()); + } + } + + String seed = uri.containsDataKey(CoreAttributeConstants.A_SOURCE_TAG) + ? uri.getSourceTag() + : "-"; + + // 2019-04-29T21:14:13.139Z 1 53 dns:www.indiewire.com P https://www.indiewire.com/2019/04/gemini-man-trailer-will-smith-ang-lee-1202126973/ text/dns #015 20190429211412388+219 sha1:WAY2F6QNMMIXRR2NWGQH2COJIAKRQO2S - - {"warcFilename":"WEB-20190429211413120-00000-48039~10.30.67.32~6440.warc.gz","warcFileOffset":1530} + ydlLogger.info( + uri.getFetchStatus() + + " " + length + + " " + MimetypeUtils.truncate(uri.getContentType()) + + " " + uri.getContentDigestSchemeString() + + " " + ydlAnnotation + + " " + ArchiveUtils.get17DigitDate(uri.getFetchBeginTime()) + + " " + uri + + " " + containingPageUri(uri) + + " " + seed); + } + + protected String containingPageUri(CrawlURI uri) { + String u = (String) uri.getData().get("containingPage"); + if (u != null) { + return u; + } else { + return uri.getVia().toString(); + } + } + + protected void doRedirectInheritance(CrawlURI uri, String ydlAnnotation) { + for (CrawlURI link: uri.getOutLinks()) { + if (link.getLastHop() == "R") { + link.getAnnotations().add(ydlAnnotation); + link.getData().put("containingPage", containingPageUri(uri)); + } + } + } + + /** + * + * @param uri + * @return list of json blobs returned by {@code youtube-dl --dump-json}, or + * empty list if no videos found, or failure + */ + @SuppressWarnings("unchecked") + protected List runYoutubeDL(CrawlURI uri) { + /* + * --format=best + * + * best: Select the best quality format represented by a single file + * with video and audio. + * https://github.com/ytdl-org/youtube-dl/blob/master/README.md#format-selection + */ + ProcessBuilder pb = new ProcessBuilder("youtube-dl", "--ignore-config", + "--simulate", "--dump-json", "--format=best", uri.toString()); + logger.fine("running " + pb.command()); + + Process proc = null; + try { + proc = pb.start(); + } catch (IOException e) { + logger.log(Level.WARNING, "youtube-dl failed " + pb.command(), e); + return (List) Collections.EMPTY_LIST; + } + + String stdout = null; + String stderr = null; + try { + stdout = readToEnd( + new InputStreamReader(proc.getInputStream(), "UTF-8")); + stderr = readToEnd( + new InputStreamReader(proc.getErrorStream(), "UTF-8")); + } catch (IOException e) { + logger.log(Level.WARNING, + "problem reading output from youtube-dl " + pb.command(), + e); + return (List) Collections.EMPTY_LIST; + } + + try { + if (proc.waitFor() != 0) { + if (!stderr.contains("ERROR: Unsupported URL:") + && !stderr.contains("ERROR: There's no video in this tweet")) { + logger.warning("youtube-dl exited with status " + + proc.waitFor() + " " + pb.command() + + "\n=== stdout ===\n" + stdout + + "\n=== stderr ===\n" + stderr); + } + // else it just didn't find a video + return (List) Collections.EMPTY_LIST; + } + } catch (InterruptedException e) { + proc.destroyForcibly(); + } + + // logger.info("youtube-dl stdout:\n" + stdout); + // logger.info("youtube-dl stderr:\n" + stderr); + + ArrayList ydlJsons = new ArrayList(); + JsonStreamParser parser = new JsonStreamParser(stdout); + try { + while (parser.hasNext()) { + ydlJsons.add((JsonObject) parser.next()); + } + } catch (JsonParseException e) { + logger.log(Level.WARNING, + "problem parsing json from youtube-dl " + pb.command() + + "\n=== stdout ===\n" + stdout + + "\n=== stderr ===\n" + stderr, + e); + return (List) Collections.EMPTY_LIST; + } + + return ydlJsons; + } + + /** + * Run youtube-dl on html 200 responses. + * + * @see ExtractorHTML#shouldExtract(CrawlURI) + */ + @Override + protected boolean shouldProcess(CrawlURI uri) { + // We have some special sauce (not actually extraction) to apply to + // "youtube-dl"-annotated urls, see extract(). + if (findYdlAnnotation(uri) != null) { + return true; + } + + // Otherwise, check if we want to run youtube-dl on the url. + return shouldExtract(uri); + } + + /** + * Returns {@code true} if we should run youtube-dl on this url. We run + * youtube-dl on html 200s that are not too huge. + */ + protected boolean shouldExtract(CrawlURI uri) { + if (uri.getFetchStatus() != 200) { + return false; + } + + // see https://github.com/internetarchive/brozzler/blob/65fad5e8b/brozzler/ydl.py#L48 + if (uri.getContentLength() <= 0 || uri.getContentLength() >= 200000000) { + return false; + } + + String mime = uri.getContentType().toLowerCase(); + if (mime.startsWith("text/html") + || mime.startsWith("application/xhtml") + || mime.startsWith("text/vnd.wap.wml") + || mime.startsWith("application/vnd.wap.wml") + || mime.startsWith("application/vnd.wap.xhtml")) { + return true; + } + + return false; + } +} diff --git a/engine/src/main/java/org/archive/crawler/io/UriProcessingFormatter.java b/engine/src/main/java/org/archive/crawler/io/UriProcessingFormatter.java index 5c12e4ae..3af99d35 100644 --- a/engine/src/main/java/org/archive/crawler/io/UriProcessingFormatter.java +++ b/engine/src/main/java/org/archive/crawler/io/UriProcessingFormatter.java @@ -151,7 +151,6 @@ extends Formatter implements Preformatter, CoreAttributeConstants { } if (logExtraInfo) { - // XXX would we rather have "-" if info's empty? buffer.append(" ").append(curi.getExtraInfo()); } From 21cfd4b73d83823afc1f273b6feb2ad5f7fc6fe5 Mon Sep 17 00:00:00 2001 From: Noah Levitt Date: Tue, 30 Apr 2019 16:16:25 -0700 Subject: [PATCH 2/9] making everything work --- .../modules/extractor/ExtractorYoutubeDL.java | 121 ++++++++++++------ 1 file changed, 83 insertions(+), 38 deletions(-) diff --git a/contrib/src/main/java/org/archive/modules/extractor/ExtractorYoutubeDL.java b/contrib/src/main/java/org/archive/modules/extractor/ExtractorYoutubeDL.java index 16a2ce87..5e4b8a32 100644 --- a/contrib/src/main/java/org/archive/modules/extractor/ExtractorYoutubeDL.java +++ b/contrib/src/main/java/org/archive/modules/extractor/ExtractorYoutubeDL.java @@ -1,3 +1,22 @@ +/* + * This file is part of the Heritrix web crawler (crawler.archive.org). + * + * Licensed to the Internet Archive (IA) by one or more individual + * contributors. + * + * The IA licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + package org.archive.modules.extractor; import java.io.IOException; @@ -24,11 +43,34 @@ import com.google.gson.JsonObject; import com.google.gson.JsonParseException; import com.google.gson.JsonStreamParser; +/** + * Extracts links to media by running youtube-dl in a subprocess. Runs only on + * html. + * + *

+ * Keeps a log of media captured as a result of youtube-dl extraction. The + * format of the log is as follows: + * + * 

[timestamp] [media-http-status] [media-length] [media-mimetype] [media-digest] [media-timestamp] [media-url] [annotation] [containing-page-digest] [containing-page-timestamp] [containing-page-timestamp] [seed-url]
+ * + *

+ * The annotation field looks like {@code "youtube-dl:1/3"}. In this example, + * "3" is the number of media urls youtube-dl discovered on the page, and "1" is + * the index of this media within the page. The intention is to use this for + * playback. The rest of the fields included in the log were also chosen to + * support creation of an index of media by containing page, to be used for + * playback. + * + * @author nlevitt + */ public class ExtractorYoutubeDL extends Extractor implements Lifecycle { - private static Logger logger = Logger.getLogger(ExtractorYoutubeDL.class.getName()); - + + protected static final String YDL_CONTAINING_PAGE_DIGEST = "ydl-containing-page-digest"; + protected static final String YDL_CONTAINING_PAGE_TIMESTAMP = "ydl-containing-page-timestamp"; + protected static final String YDL_CONTAINING_PAGE_URI = "ydl-containing-page-uri"; + protected transient Logger ydlLogger = null; protected CrawlerLoggerModule crawlerLoggerModule; @@ -39,7 +81,7 @@ public class ExtractorYoutubeDL extends Extractor implements Lifecycle { public void setCrawlerLoggerModule(CrawlerLoggerModule crawlerLoggerModule) { this.crawlerLoggerModule = crawlerLoggerModule; } - + @Override public void start() { if (!isRunning) { @@ -103,12 +145,22 @@ public class ExtractorYoutubeDL extends Extractor implements Lifecycle { UURI dest = UURIFactory.getInstance(uri.getUURI(), videoUrl); CrawlURI link = uri.createCrawlURI(dest, LinkContext.EMBED_MISC, Hop.EMBED); + + // annotation String annotation = "youtube-dl:1/1"; if (!json.get("playlist_index").isJsonNull()) { annotation = "youtube-dl:" + json.get("playlist_index") + "/" + json.get("n_entries"); } link.getAnnotations().add(annotation); + + // save info unambiguously identifying containing page capture + link.getData().put(YDL_CONTAINING_PAGE_URI, uri.toString()); + link.getData().put(YDL_CONTAINING_PAGE_TIMESTAMP, + ArchiveUtils.get17DigitDate(uri.getFetchBeginTime())); + link.getData().put(YDL_CONTAINING_PAGE_DIGEST, + uri.getContentDigestSchemeString()); + uri.getOutLinks().add(link); } catch (URIException e) { logUriError(e, uri.getUURI(), videoUrl); @@ -136,46 +188,43 @@ public class ExtractorYoutubeDL extends Extractor implements Lifecycle { } else { if (uri.getContentSize() > 0) { length = Long.toString(uri.getContentSize()); - } + } } - String seed = uri.containsDataKey(CoreAttributeConstants.A_SOURCE_TAG) + String seed = uri.containsDataKey(CoreAttributeConstants.A_SOURCE_TAG) ? uri.getSourceTag() : "-"; - // 2019-04-29T21:14:13.139Z 1 53 dns:www.indiewire.com P https://www.indiewire.com/2019/04/gemini-man-trailer-will-smith-ang-lee-1202126973/ text/dns #015 20190429211412388+219 sha1:WAY2F6QNMMIXRR2NWGQH2COJIAKRQO2S - - {"warcFilename":"WEB-20190429211413120-00000-48039~10.30.67.32~6440.warc.gz","warcFileOffset":1530} ydlLogger.info( uri.getFetchStatus() + " " + length + " " + MimetypeUtils.truncate(uri.getContentType()) + " " + uri.getContentDigestSchemeString() - + " " + ydlAnnotation + " " + ArchiveUtils.get17DigitDate(uri.getFetchBeginTime()) + " " + uri - + " " + containingPageUri(uri) + + " " + ydlAnnotation + + " " + uri.getData().get(YDL_CONTAINING_PAGE_DIGEST) + + " " + uri.getData().get(YDL_CONTAINING_PAGE_TIMESTAMP) + + " " + uri.getData().get(YDL_CONTAINING_PAGE_URI) + " " + seed); } - protected String containingPageUri(CrawlURI uri) { - String u = (String) uri.getData().get("containingPage"); - if (u != null) { - return u; - } else { - return uri.getVia().toString(); - } - } - protected void doRedirectInheritance(CrawlURI uri, String ydlAnnotation) { for (CrawlURI link: uri.getOutLinks()) { - if (link.getLastHop() == "R") { + if ("R".equals(link.getLastHop())) { link.getAnnotations().add(ydlAnnotation); - link.getData().put("containingPage", containingPageUri(uri)); + link.getData().put(YDL_CONTAINING_PAGE_URI, + uri.getData().get(YDL_CONTAINING_PAGE_URI)); + link.getData().put(YDL_CONTAINING_PAGE_TIMESTAMP, + uri.getData().get(YDL_CONTAINING_PAGE_TIMESTAMP)); + link.getData().put(YDL_CONTAINING_PAGE_DIGEST, + uri.getData().get(YDL_CONTAINING_PAGE_DIGEST)); } } } /** - * + * * @param uri * @return list of json blobs returned by {@code youtube-dl --dump-json}, or * empty list if no videos found, or failure @@ -184,7 +233,7 @@ public class ExtractorYoutubeDL extends Extractor implements Lifecycle { protected List runYoutubeDL(CrawlURI uri) { /* * --format=best - * + * * best: Select the best quality format represented by a single file * with video and audio. * https://github.com/ytdl-org/youtube-dl/blob/master/README.md#format-selection @@ -217,23 +266,22 @@ public class ExtractorYoutubeDL extends Extractor implements Lifecycle { try { if (proc.waitFor() != 0) { - if (!stderr.contains("ERROR: Unsupported URL:") - && !stderr.contains("ERROR: There's no video in this tweet")) { - logger.warning("youtube-dl exited with status " + /* + * youtube-dl is noisy when it fails to find a video. I guess + * the assumption is that you're running it on pages you know + * have videos. We could be hiding real errors in some cases + * but it's just too much noise to log this at WARNING level. + */ + logger.fine("youtube-dl exited with status " + proc.waitFor() + " " + pb.command() + "\n=== stdout ===\n" + stdout + "\n=== stderr ===\n" + stderr); - } - // else it just didn't find a video return (List) Collections.EMPTY_LIST; } } catch (InterruptedException e) { proc.destroyForcibly(); } - // logger.info("youtube-dl stdout:\n" + stdout); - // logger.info("youtube-dl stderr:\n" + stderr); - ArrayList ydlJsons = new ArrayList(); JsonStreamParser parser = new JsonStreamParser(stdout); try { @@ -241,7 +289,9 @@ public class ExtractorYoutubeDL extends Extractor implements Lifecycle { ydlJsons.add((JsonObject) parser.next()); } } catch (JsonParseException e) { - logger.log(Level.WARNING, + // sometimes we get no output at all from youtube-dl, which + // manifests as a JsonIOException + logger.log(Level.FINE, "problem parsing json from youtube-dl " + pb.command() + "\n=== stdout ===\n" + stdout + "\n=== stderr ===\n" + stderr, @@ -252,15 +302,10 @@ public class ExtractorYoutubeDL extends Extractor implements Lifecycle { return ydlJsons; } - /** - * Run youtube-dl on html 200 responses. - * - * @see ExtractorHTML#shouldExtract(CrawlURI) - */ @Override protected boolean shouldProcess(CrawlURI uri) { // We have some special sauce (not actually extraction) to apply to - // "youtube-dl"-annotated urls, see extract(). + // "youtube-dl"-annotated urls. See extract(). if (findYdlAnnotation(uri) != null) { return true; } @@ -277,12 +322,12 @@ public class ExtractorYoutubeDL extends Extractor implements Lifecycle { if (uri.getFetchStatus() != 200) { return false; } - + // see https://github.com/internetarchive/brozzler/blob/65fad5e8b/brozzler/ydl.py#L48 if (uri.getContentLength() <= 0 || uri.getContentLength() >= 200000000) { return false; } - + String mime = uri.getContentType().toLowerCase(); if (mime.startsWith("text/html") || mime.startsWith("application/xhtml") From 1bd8b713c6b5344c18950827aeac1a910d3679e3 Mon Sep 17 00:00:00 2001 From: Noah Levitt Date: Tue, 30 Apr 2019 16:17:35 -0700 Subject: [PATCH 3/9] quiet org.mortbay.log (jetty?) logging MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There is already a clause for this in logging.properties, but it's using log4j. It was dumping stack traces every time the client was dubious of heritrix's self-signed certificate. Why do we have so many identical log4j.xml's? 🤷‍♂️ --- commons/src/test/resources/log4j.xml | 4 ++++ contrib/src/main/resources/log4j.xml | 4 ++++ contrib/src/test/resources/log4j.xml | 4 ++++ dist/src/test/resources/log4j.xml | 4 ++++ engine/src/test/resources/log4j.xml | 4 ++++ modules/src/test/resources/log4j.xml | 4 ++++ 6 files changed, 24 insertions(+) diff --git a/commons/src/test/resources/log4j.xml b/commons/src/test/resources/log4j.xml index eddf51e2..475b130e 100644 --- a/commons/src/test/resources/log4j.xml +++ b/commons/src/test/resources/log4j.xml @@ -11,6 +11,10 @@ + + + + diff --git a/contrib/src/main/resources/log4j.xml b/contrib/src/main/resources/log4j.xml index eddf51e2..475b130e 100644 --- a/contrib/src/main/resources/log4j.xml +++ b/contrib/src/main/resources/log4j.xml @@ -11,6 +11,10 @@ + + + + diff --git a/contrib/src/test/resources/log4j.xml b/contrib/src/test/resources/log4j.xml index eddf51e2..475b130e 100644 --- a/contrib/src/test/resources/log4j.xml +++ b/contrib/src/test/resources/log4j.xml @@ -11,6 +11,10 @@ + + + + diff --git a/dist/src/test/resources/log4j.xml b/dist/src/test/resources/log4j.xml index eddf51e2..475b130e 100644 --- a/dist/src/test/resources/log4j.xml +++ b/dist/src/test/resources/log4j.xml @@ -11,6 +11,10 @@ + + + + diff --git a/engine/src/test/resources/log4j.xml b/engine/src/test/resources/log4j.xml index eddf51e2..475b130e 100644 --- a/engine/src/test/resources/log4j.xml +++ b/engine/src/test/resources/log4j.xml @@ -11,6 +11,10 @@ + + + + diff --git a/modules/src/test/resources/log4j.xml b/modules/src/test/resources/log4j.xml index eddf51e2..475b130e 100644 --- a/modules/src/test/resources/log4j.xml +++ b/modules/src/test/resources/log4j.xml @@ -11,6 +11,10 @@ + + + + From b7de2bd2aebd85c092e9daf81ee8fabddb6a6e79 Mon Sep 17 00:00:00 2001 From: Noah Levitt Date: Tue, 30 Apr 2019 16:20:35 -0700 Subject: [PATCH 4/9] nothing private ever --- .../java/org/archive/modules/extractor/ExtractorYoutubeDL.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contrib/src/main/java/org/archive/modules/extractor/ExtractorYoutubeDL.java b/contrib/src/main/java/org/archive/modules/extractor/ExtractorYoutubeDL.java index 5e4b8a32..fc7d5ba6 100644 --- a/contrib/src/main/java/org/archive/modules/extractor/ExtractorYoutubeDL.java +++ b/contrib/src/main/java/org/archive/modules/extractor/ExtractorYoutubeDL.java @@ -139,7 +139,7 @@ public class ExtractorYoutubeDL extends Extractor implements Lifecycle { } } - private void addVideoOutlink(CrawlURI uri, JsonObject json, + protected void addVideoOutlink(CrawlURI uri, JsonObject json, String videoUrl) { try { UURI dest = UURIFactory.getInstance(uri.getUURI(), videoUrl); From 37fb6f6b7b176325ebc8f916d73ed8552092ddcd Mon Sep 17 00:00:00 2001 From: Noah Levitt Date: Tue, 30 Apr 2019 16:20:53 -0700 Subject: [PATCH 5/9] do not drop any `CrawlURI.data` between processing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Without this change (or other measures), we sometimes get nulls in the ExtractorYoutubeDL log for containing page information. We'll run this on QA for a while and see if it causes any problems. nlevitt [1:59 PM] https://github.com/internetarchive/heritrix3/blob/master/modules/src/main/java/org/archive/modules/CrawlURI.java#L878 drops some stuff from `CrawlURI.data` after processing a uri, even if it needs to be processed again there is a list of keys that shouldn’t be dropped (`persistentKeys`), but it is final and private so if you’re writing your own heritrix module and you want to keep some information in CrawlURI.data, it usually works, except when the url is processed more than once (like when it needs a prereq like robots.txt the first time) in practice it seems that most data is persisted, that is, most commonly used keys are in `persistentKeys` in a crawl with pretty standard configuration i’m mostly seeing `prerequisite-uri` dropped and occasionally `fetch-completed-time` and `fetch-began-time` being dropped i’m highly skeptical of the value of dropping keys at all and i’m tempted to get rid of this entirely, make all the keys persistent in other words soliciting feedback (edited) anjackson [2:39 PM] My immediate reaction is HARD AGREE. It looks like Really Old Code though (https://github.com/internetarchive/heritrix3/blame/7d3eff5269142c77fa4b988396153f4c29d16caa/modules/src/main/java/org/archive/modules/CrawlURI.java#L878) so the reasons for doing so may have been lost in time. Hm, looking at usage: https://github.com/internetarchive/heritrix3/blob/a60b2ef3875ad47f57b0c6b3c0b19f86c40a12f7/engine/src/main/java/org/archive/crawler/frontier/WorkQueueFrontier.java#L954-L955 engine/src/main/java/org/archive/crawler/frontier/WorkQueueFrontier.java:954-955 curi.processingCleanup(); // lose state that shouldn't burden // retry I guess there's a concern that there may be state in there that is set during a fetch and may cause problems if the same CrawlURI is deferred? But I'm not aware of anything in the fetch chain that behaves like that. nlevitt [3:02 PM] oh, i missed `CrawlURI.addDataPersistentMember(String)` et al. still... --- .../java/org/archive/modules/CrawlURI.java | 69 +------------------ .../recrawl/FetchHistoryProcessor.java | 1 - .../modules/recrawl/PersistLogProcessor.java | 2 +- .../recrawl/PersistStoreProcessor.java | 2 +- 4 files changed, 3 insertions(+), 71 deletions(-) diff --git a/modules/src/main/java/org/archive/modules/CrawlURI.java b/modules/src/main/java/org/archive/modules/CrawlURI.java index 46a935b2..92d2870f 100644 --- a/modules/src/main/java/org/archive/modules/CrawlURI.java +++ b/modules/src/main/java/org/archive/modules/CrawlURI.java @@ -31,9 +31,6 @@ import static org.archive.modules.CoreAttributeConstants.A_HTTP_RESPONSE_HEADERS import static org.archive.modules.CoreAttributeConstants.A_NONFATAL_ERRORS; import static org.archive.modules.CoreAttributeConstants.A_PREREQUISITE_URI; import static org.archive.modules.CoreAttributeConstants.A_SOURCE_TAG; -import static org.archive.modules.CoreAttributeConstants.A_SUBMIT_DATA; -import static org.archive.modules.CoreAttributeConstants.A_SUBMIT_ENCTYPE; -import static org.archive.modules.CoreAttributeConstants.A_WARC_RESPONSE_HEADERS; import static org.archive.modules.SchedulingConstants.NORMAL; import static org.archive.modules.fetcher.FetchStatusCodes.S_BLOCKED_BY_CUSTOM_PROCESSOR; import static org.archive.modules.fetcher.FetchStatusCodes.S_BLOCKED_BY_USER; @@ -76,7 +73,6 @@ import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Set; -import java.util.concurrent.CopyOnWriteArrayList; import java.util.logging.Level; import java.util.logging.Logger; @@ -248,15 +244,6 @@ implements Reporter, Serializable, OverlayContext, Comparable { * buggy */ protected long ordinal; - - /** - * Array to hold keys of data members that persist across URI processings. - * Any key mentioned in this list will not be cleared out at the end - * of a pass down the processing chain. - */ - private static final Collection persistentKeys - = new CopyOnWriteArrayList( - new String [] {A_CREDENTIALS_KEY, A_HTTP_AUTH_CHALLENGES, A_SUBMIT_DATA, A_WARC_RESPONSE_HEADERS, A_ANNOTATIONS, A_SUBMIT_ENCTYPE}); /** maximum length for pathFromSeed/hopsPath; longer truncated with leading counter **/ private static final int MAX_HOPS_DISPLAYED = 50; @@ -875,8 +862,6 @@ implements Reporter, Serializable, OverlayContext, Comparable { this.contentLength = UNCALCULATED; // Clear 'links extracted' flag. this.linkExtractorFinished = false; - // Clean the data map of all but registered permanent members. - this.data = getPersistentDataMap(); extraInfo = null; outLinks = null; @@ -886,23 +871,6 @@ implements Reporter, Serializable, OverlayContext, Comparable { // XXX er uh surprised this wasn't here before? fetchType = FetchType.UNKNOWN; } - - public Map getPersistentDataMap() { - if (data == null) { - return null; - } - Map result = new HashMap(getData()); - Set retain = new HashSet(persistentKeys); - - if (containsDataKey(A_HERITABLE_KEYS)) { - @SuppressWarnings("unchecked") - HashSet heritable = (HashSet)getData().get(A_HERITABLE_KEYS); - retain.addAll(heritable); - } - - result.keySet().retainAll(retain); - return result; - } /** * @return Credential avatars. Null if none set. @@ -1132,39 +1100,6 @@ implements Reporter, Serializable, OverlayContext, Comparable { } return (UURI)getData().get(A_HTML_BASE); } - - public static Collection getPersistentDataKeys() { - return persistentKeys; - } - - /** - * Add the key of items you want to persist across - * processings. - * @param s Key to add. - */ - public void addPersistentDataMapKey(String s) { - if (!persistentKeys.contains(s)) { - addDataPersistentMember(s); - } - } - - /** - * Add the key of data map items you want to persist across - * processings. - * @param key Key to add. - */ - public static void addDataPersistentMember(String key) { - persistentKeys.add(key); - } - - /** - * Remove the key from those data map members persisted. - * @param key Key to remove. - * @return True if list contained the element. - */ - public static boolean removeDataPersistentMember(String key) { - return persistentKeys.remove(key); - } private void writeObject(ObjectOutputStream stream) throws IOException { stream.defaultWriteObject(); @@ -1782,9 +1717,7 @@ implements Reporter, Serializable, OverlayContext, Comparable { return containsDataKey(A_FORCE_RETIRE) && (Boolean)getData().get(A_FORCE_RETIRE); } - - - + protected JSONObject extraInfo; public JSONObject getExtraInfo() { diff --git a/modules/src/main/java/org/archive/modules/recrawl/FetchHistoryProcessor.java b/modules/src/main/java/org/archive/modules/recrawl/FetchHistoryProcessor.java index 7bcd5ccf..889fa05c 100644 --- a/modules/src/main/java/org/archive/modules/recrawl/FetchHistoryProcessor.java +++ b/modules/src/main/java/org/archive/modules/recrawl/FetchHistoryProcessor.java @@ -67,7 +67,6 @@ public class FetchHistoryProcessor extends Processor { @Override protected void innerProcess(CrawlURI puri) throws InterruptedException { CrawlURI curi = (CrawlURI) puri; - curi.addPersistentDataMapKey(A_FETCH_HISTORY); HashMap latestFetch = new HashMap(); // save status diff --git a/modules/src/main/java/org/archive/modules/recrawl/PersistLogProcessor.java b/modules/src/main/java/org/archive/modules/recrawl/PersistLogProcessor.java index d03cb5af..f5dada4d 100644 --- a/modules/src/main/java/org/archive/modules/recrawl/PersistLogProcessor.java +++ b/modules/src/main/java/org/archive/modules/recrawl/PersistLogProcessor.java @@ -96,7 +96,7 @@ implements Checkpointable, Lifecycle { protected void innerProcess(CrawlURI curi) { log.writeLine(persistKeyFor(curi), " ", new String(Base64.encodeBase64( - SerializationUtils.serialize((Serializable)curi.getPersistentDataMap())))); + SerializationUtils.serialize((Serializable)curi.getData())))); } public void startCheckpoint(Checkpoint checkpointInProgress) {} diff --git a/modules/src/main/java/org/archive/modules/recrawl/PersistStoreProcessor.java b/modules/src/main/java/org/archive/modules/recrawl/PersistStoreProcessor.java index 9bb7da92..73468c3b 100644 --- a/modules/src/main/java/org/archive/modules/recrawl/PersistStoreProcessor.java +++ b/modules/src/main/java/org/archive/modules/recrawl/PersistStoreProcessor.java @@ -40,7 +40,7 @@ public class PersistStoreProcessor extends PersistOnlineProcessor @Override protected void innerProcess(CrawlURI curi) throws InterruptedException { - store.put(persistKeyFor(curi),curi.getPersistentDataMap()); + store.put(persistKeyFor(curi), curi.getData()); } @Override From b4aa3c9511456eb8722010166c01b9db885d0bd1 Mon Sep 17 00:00:00 2001 From: Noah Levitt Date: Thu, 2 May 2019 15:46:14 -0700 Subject: [PATCH 6/9] read stderr and stdout in separate threads... ... to avoid hanging see https://github.com/internetarchive/heritrix3/pull/257/files#r279990349 thanks Alex! --- .../modules/extractor/ExtractorYoutubeDL.java | 34 +++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-) diff --git a/contrib/src/main/java/org/archive/modules/extractor/ExtractorYoutubeDL.java b/contrib/src/main/java/org/archive/modules/extractor/ExtractorYoutubeDL.java index fc7d5ba6..67328209 100644 --- a/contrib/src/main/java/org/archive/modules/extractor/ExtractorYoutubeDL.java +++ b/contrib/src/main/java/org/archive/modules/extractor/ExtractorYoutubeDL.java @@ -25,6 +25,11 @@ import java.io.Reader; import java.util.ArrayList; import java.util.Collections; import java.util.List; +import java.util.concurrent.Callable; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; import java.util.logging.Level; import java.util.logging.Logger; @@ -51,7 +56,7 @@ import com.google.gson.JsonStreamParser; * Keeps a log of media captured as a result of youtube-dl extraction. The * format of the log is as follows: * - * 

[timestamp] [media-http-status] [media-length] [media-mimetype] [media-digest] [media-timestamp] [media-url] [annotation] [containing-page-digest] [containing-page-timestamp] [containing-page-timestamp] [seed-url]
+ * 
[timestamp] [media-http-status] [media-length] [media-mimetype] [media-digest] [media-timestamp] [media-url] [annotation] [containing-page-digest] [containing-page-timestamp] [containing-page-url] [seed-url]
* *

* The annotation field looks like {@code "youtube-dl:1/3"}. In this example, @@ -107,6 +112,31 @@ public class ExtractorYoutubeDL extends Extractor implements Lifecycle { } } + // see https://github.com/internetarchive/heritrix3/pull/257/files#r279990349 + protected String readToEndInThread(Reader reader) throws IOException { + ExecutorService threadPool = Executors.newSingleThreadExecutor(); + Future future = threadPool.submit(new Callable() { + @Override + public String call() throws IOException { + return readToEnd(reader); + } + }); + + try { + return future.get(); + } catch (InterruptedException e) { + throw new IOException(e); // :shrug: + } catch (ExecutionException e) { + if (e.getCause() instanceof IOException) { + throw (IOException) e.getCause(); + } else { + throw new IOException(e); + } + } finally { + threadPool.shutdown(); + } + } + /** * - If {@code uri} is annotated "youtube-dl" and is a 3xx (redirect), * find the redirect among the outlinks and add the "youtube-dl" @@ -255,7 +285,7 @@ public class ExtractorYoutubeDL extends Extractor implements Lifecycle { try { stdout = readToEnd( new InputStreamReader(proc.getInputStream(), "UTF-8")); - stderr = readToEnd( + stderr = readToEndInThread( new InputStreamReader(proc.getErrorStream(), "UTF-8")); } catch (IOException e) { logger.log(Level.WARNING, From 7c66da7be7b8a7dfe82819ab578976b1990d6f6d Mon Sep 17 00:00:00 2001 From: Noah Levitt Date: Thu, 2 May 2019 15:48:46 -0700 Subject: [PATCH 7/9] whoops, better spawn the thread first --- .../org/archive/modules/extractor/ExtractorYoutubeDL.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contrib/src/main/java/org/archive/modules/extractor/ExtractorYoutubeDL.java b/contrib/src/main/java/org/archive/modules/extractor/ExtractorYoutubeDL.java index 67328209..49d11fce 100644 --- a/contrib/src/main/java/org/archive/modules/extractor/ExtractorYoutubeDL.java +++ b/contrib/src/main/java/org/archive/modules/extractor/ExtractorYoutubeDL.java @@ -283,10 +283,10 @@ public class ExtractorYoutubeDL extends Extractor implements Lifecycle { String stdout = null; String stderr = null; try { - stdout = readToEnd( - new InputStreamReader(proc.getInputStream(), "UTF-8")); stderr = readToEndInThread( new InputStreamReader(proc.getErrorStream(), "UTF-8")); + stdout = readToEnd( + new InputStreamReader(proc.getInputStream(), "UTF-8")); } catch (IOException e) { logger.log(Level.WARNING, "problem reading output from youtube-dl " + pb.command(), From 75b33663f98f89aefad33bd6921761eea9a3d6d7 Mon Sep 17 00:00:00 2001 From: Noah Levitt Date: Mon, 6 May 2019 15:35:47 -0700 Subject: [PATCH 8/9] also annotate and log containing pages --- .../modules/extractor/ExtractorYoutubeDL.java | 47 ++++++++++++++----- 1 file changed, 35 insertions(+), 12 deletions(-) diff --git a/contrib/src/main/java/org/archive/modules/extractor/ExtractorYoutubeDL.java b/contrib/src/main/java/org/archive/modules/extractor/ExtractorYoutubeDL.java index 49d11fce..92217d45 100644 --- a/contrib/src/main/java/org/archive/modules/extractor/ExtractorYoutubeDL.java +++ b/contrib/src/main/java/org/archive/modules/extractor/ExtractorYoutubeDL.java @@ -53,18 +53,22 @@ import com.google.gson.JsonStreamParser; * html. * *

- * Keeps a log of media captured as a result of youtube-dl extraction. The - * format of the log is as follows: + * Keeps a log of containing pages and media captured as a result of youtube-dl + * extraction. The format of the log is as follows: * * 

[timestamp] [media-http-status] [media-length] [media-mimetype] [media-digest] [media-timestamp] [media-url] [annotation] [containing-page-digest] [containing-page-timestamp] [containing-page-url] [seed-url]
* *

- * The annotation field looks like {@code "youtube-dl:1/3"}. In this example, - * "3" is the number of media urls youtube-dl discovered on the page, and "1" is - * the index of this media within the page. The intention is to use this for - * playback. The rest of the fields included in the log were also chosen to - * support creation of an index of media by containing page, to be used for - * playback. + * For containing pages, all of the {@code media-*} fields have the value + * {@code "-"}, and the annotation field looks like {@code "youtube-dl:3"}, + * meaning that ExtractorYoutubeDL extracted 3 media links from the page. + * + *

+ * For media, the annotation field looks like {@code "youtube-dl:1/3"}, meaning + * this is the first of three media links extracted from the containing page. + * The intention is to use this for playback. The rest of the fields included in + * the log were also chosen to support creation of an index of media by + * containing page, to be used for playback. * * @author nlevitt */ @@ -160,11 +164,16 @@ public class ExtractorYoutubeDL extends Extractor implements Lifecycle { } } else { List ydlJsons = runYoutubeDL(uri); - for (JsonObject json: ydlJsons) { - if (json.get("url") != null) { - String videoUrl = json.get("url").getAsString(); - addVideoOutlink(uri, json, videoUrl); + if (ydlJsons != null && !ydlJsons.isEmpty()) { + for (JsonObject json: ydlJsons) { + if (json.get("url") != null) { + String videoUrl = json.get("url").getAsString(); + addVideoOutlink(uri, json, videoUrl); + } } + String annotation = "youtube-dl:" + ydlJsons.size(); + uri.getAnnotations().add(annotation); + logContainingPage(uri, annotation); } } } @@ -239,6 +248,20 @@ public class ExtractorYoutubeDL extends Extractor implements Lifecycle { + " " + seed); } + protected void logContainingPage(CrawlURI uri, String annotation) { + String seed = uri.containsDataKey(CoreAttributeConstants.A_SOURCE_TAG) + ? uri.getSourceTag() + : "-"; + + ydlLogger.info( + "- - - - - -" + + " " + annotation + + " " + uri.getContentDigestSchemeString() + + " " + ArchiveUtils.get17DigitDate(uri.getFetchBeginTime()) + + " " + uri + + " " + seed); + } + protected void doRedirectInheritance(CrawlURI uri, String ydlAnnotation) { for (CrawlURI link: uri.getOutLinks()) { if ("R".equals(link.getLastHop())) { From 99eebb81019e43615d713f495be88030673ad561 Mon Sep 17 00:00:00 2001 From: Noah Levitt Date: Tue, 7 May 2019 11:01:34 -0700 Subject: [PATCH 9/9] ugh, get it right reading stderr in a separate thread doesn't help if you wait for that thread to finish before reading stdout --- .../modules/extractor/ExtractorYoutubeDL.java | 95 ++++++++++--------- 1 file changed, 52 insertions(+), 43 deletions(-) diff --git a/contrib/src/main/java/org/archive/modules/extractor/ExtractorYoutubeDL.java b/contrib/src/main/java/org/archive/modules/extractor/ExtractorYoutubeDL.java index 92217d45..a1f2df97 100644 --- a/contrib/src/main/java/org/archive/modules/extractor/ExtractorYoutubeDL.java +++ b/contrib/src/main/java/org/archive/modules/extractor/ExtractorYoutubeDL.java @@ -23,7 +23,6 @@ import java.io.IOException; import java.io.InputStreamReader; import java.io.Reader; import java.util.ArrayList; -import java.util.Collections; import java.util.List; import java.util.concurrent.Callable; import java.util.concurrent.ExecutionException; @@ -116,31 +115,6 @@ public class ExtractorYoutubeDL extends Extractor implements Lifecycle { } } - // see https://github.com/internetarchive/heritrix3/pull/257/files#r279990349 - protected String readToEndInThread(Reader reader) throws IOException { - ExecutorService threadPool = Executors.newSingleThreadExecutor(); - Future future = threadPool.submit(new Callable() { - @Override - public String call() throws IOException { - return readToEnd(reader); - } - }); - - try { - return future.get(); - } catch (InterruptedException e) { - throw new IOException(e); // :shrug: - } catch (ExecutionException e) { - if (e.getCause() instanceof IOException) { - throw (IOException) e.getCause(); - } else { - throw new IOException(e); - } - } finally { - threadPool.shutdown(); - } - } - /** * - If {@code uri} is annotated "youtube-dl" and is a 3xx (redirect), * find the redirect among the outlinks and add the "youtube-dl" @@ -276,13 +250,52 @@ public class ExtractorYoutubeDL extends Extractor implements Lifecycle { } } + static protected class ProcessOutput { + public String stdout; + public String stderr; + } + + // read stdout in this thread, stderr in separate thread + // see https://github.com/internetarchive/heritrix3/pull/257/files#r279990349 + protected ProcessOutput readOutput(Process proc) throws IOException { + ProcessOutput output = new ProcessOutput(); + + Reader err = new InputStreamReader(proc.getErrorStream(), "UTF-8"); + InputStreamReader out = new InputStreamReader(proc.getInputStream(), "UTF-8"); + ExecutorService threadPool = Executors.newSingleThreadExecutor(); + + Future future = threadPool.submit(new Callable() { + @Override + public String call() throws IOException { + return readToEnd(err); + } + }); + + output.stdout = readToEnd(out); + + try { + output.stderr = future.get(); + } catch (InterruptedException e) { + throw new IOException(e); // :shrug: + } catch (ExecutionException e) { + if (e.getCause() instanceof IOException) { + throw (IOException) e.getCause(); + } else { + throw new IOException(e); + } + } finally { + threadPool.shutdown(); + } + + return output; + } + /** * * @param uri * @return list of json blobs returned by {@code youtube-dl --dump-json}, or * empty list if no videos found, or failure */ - @SuppressWarnings("unchecked") protected List runYoutubeDL(CrawlURI uri) { /* * --format=best @@ -300,21 +313,17 @@ public class ExtractorYoutubeDL extends Extractor implements Lifecycle { proc = pb.start(); } catch (IOException e) { logger.log(Level.WARNING, "youtube-dl failed " + pb.command(), e); - return (List) Collections.EMPTY_LIST; + return null; } - String stdout = null; - String stderr = null; + ProcessOutput output; try { - stderr = readToEndInThread( - new InputStreamReader(proc.getErrorStream(), "UTF-8")); - stdout = readToEnd( - new InputStreamReader(proc.getInputStream(), "UTF-8")); + output = readOutput(proc); } catch (IOException e) { logger.log(Level.WARNING, "problem reading output from youtube-dl " + pb.command(), e); - return (List) Collections.EMPTY_LIST; + return null; } try { @@ -327,16 +336,16 @@ public class ExtractorYoutubeDL extends Extractor implements Lifecycle { */ logger.fine("youtube-dl exited with status " + proc.waitFor() + " " + pb.command() - + "\n=== stdout ===\n" + stdout - + "\n=== stderr ===\n" + stderr); - return (List) Collections.EMPTY_LIST; + + "\n=== stdout ===\n" + output.stdout + + "\n=== stderr ===\n" + output.stderr); + return null; } } catch (InterruptedException e) { proc.destroyForcibly(); } - ArrayList ydlJsons = new ArrayList(); - JsonStreamParser parser = new JsonStreamParser(stdout); + List ydlJsons = new ArrayList(); + JsonStreamParser parser = new JsonStreamParser(output.stdout); try { while (parser.hasNext()) { ydlJsons.add((JsonObject) parser.next()); @@ -346,10 +355,10 @@ public class ExtractorYoutubeDL extends Extractor implements Lifecycle { // manifests as a JsonIOException logger.log(Level.FINE, "problem parsing json from youtube-dl " + pb.command() - + "\n=== stdout ===\n" + stdout - + "\n=== stderr ===\n" + stderr, + + "\n=== stdout ===\n" + output.stdout + + "\n=== stderr ===\n" + output.stderr, e); - return (List) Collections.EMPTY_LIST; + return null; } return ydlJsons;