Skip to content

Commit 0318cad

Browse files
author
epriestley
committed
Fix two issues with audio macros
Summary: Fixes T3887. Two issues: - Macros were generating entirely before the render cache, so audio macros worked fine in previews and the first time the cache was populated, but not afterward. - Instead, parse them before the cache but drop them in after the cache. Clean up all the file querying, too. This makes cached remarkup generate the correct audio beahviors. - Safari sends an HTTP request with a "Range" header, and expects a "206 Partial Content" response. If we don't give it one, it sometimes has trouble figuring out how long a piece of audio is (mostly for longer clips? Or mostly for MP3s?). I'm not exactly sure what triggers it. The net effect is that "loop" does not work when Safari gets confused. While looping a short "quack.wav" worked fine, longer MP3s didn't loop. - Supporting "Range" and "206 Partial Content", which is straightforward, fixes this problem. Test Plan: - Viewed a page with lots of different cached audio macros and lots of different uncached preview audio macros, they all rendered correctly and played audio. - Viewed a macro with a long MP3 audio loop in Safari. Verified it looped after it completed. Used Charles to check that the server received and responded to the "Range" header correctly. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T3887 Differential Revision: https://secure.phabricator.com/D7166
1 parent b4cc990 commit 0318cad

File tree

3 files changed

+143
-60
lines changed

3 files changed

+143
-60
lines changed

‎src/aphront/response/AphrontFileResponse.php

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@ final class AphrontFileResponse extends AphrontResponse {
88
private $content;
99
private $mimeType;
1010
private $download;
11+
private $rangeMin;
12+
private $rangeMax;
1113

1214
public function setDownload($download) {
1315
$download = preg_replace('/[^A-Za-z0-9_.-]/', '_', $download);
@@ -37,15 +39,33 @@ public function setContent($content) {
3739
}
3840

3941
public function buildResponseString() {
40-
return $this->content;
42+
if ($this->rangeMin || $this->rangeMax) {
43+
$length = ($this->rangeMax - $this->rangeMin) + 1;
44+
return substr($this->content, $this->rangeMin, $length);
45+
} else {
46+
return $this->content;
47+
}
48+
}
49+
50+
public function setRange($min, $max) {
51+
$this->rangeMin = $min;
52+
$this->rangeMax = $max;
53+
return $this;
4154
}
4255

4356
public function getHeaders() {
4457
$headers = array(
4558
array('Content-Type', $this->getMimeType()),
46-
array('Content-Length', strlen($this->content)),
59+
array('Content-Length', strlen($this->buildResponseString())),
4760
);
4861

62+
if ($this->rangeMin || $this->rangeMax) {
63+
$len = strlen($this->content);
64+
$min = $this->rangeMin;
65+
$max = $this->rangeMax;
66+
$headers[] = array('Content-Range', "bytes {$min}-{$max}/{$len}");
67+
}
68+
4969
if (strlen($this->getDownload())) {
5070
$headers[] = array('X-Download-Options', 'noopen');
5171

‎src/applications/files/controller/PhabricatorFileDataController.php

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,20 @@ public function processRequest() {
4141
$response->setContent($data);
4242
$response->setCacheDurationInSeconds(60 * 60 * 24 * 30);
4343

44+
// NOTE: It's important to accept "Range" requests when playing audio.
45+
// If we don't, Safari has difficulty figuring out how long sounds are
46+
// and glitches when trying to loop them. In particular, Safari sends
47+
// an initial request for bytes 0-1 of the audio file, and things go south
48+
// if we can't respond with a 206 Partial Content.
49+
$range = $request->getHTTPHeader('range');
50+
if ($range) {
51+
$matches = null;
52+
if (preg_match('/^bytes=(\d+)-(\d+)$/', $range, $matches)) {
53+
$response->setHTTPResponseCode(206);
54+
$response->setRange((int)$matches[1], (int)$matches[2]);
55+
}
56+
}
57+
4458
$is_viewable = $file->isViewableInBrowser();
4559
$force_download = $request->getExists('download');
4660

‎src/applications/macro/remarkup/PhabricatorRemarkupRuleImageMacro.php

Lines changed: 107 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,9 @@
66
final class PhabricatorRemarkupRuleImageMacro
77
extends PhutilRemarkupRule {
88

9-
private $images;
9+
private $macros;
10+
11+
const KEY_RULE_MACRO = 'rule.macro';
1012

1113
public function apply($text) {
1214
return preg_replace_callback(
@@ -16,102 +18,149 @@ public function apply($text) {
1618
}
1719

1820
public function markupImageMacro($matches) {
19-
if ($this->images === null) {
20-
$this->images = array();
21+
if ($this->macros === null) {
22+
$this->macros = array();
2123

2224
$viewer = $this->getEngine()->getConfig('viewer');
2325
$rows = id(new PhabricatorMacroQuery())
2426
->setViewer($viewer)
2527
->withStatus(PhabricatorMacroQuery::STATUS_ACTIVE)
2628
->execute();
27-
foreach ($rows as $row) {
28-
$spec = array(
29-
'image' => $row->getFilePHID(),
30-
);
31-
32-
$behavior_none = PhabricatorFileImageMacro::AUDIO_BEHAVIOR_NONE;
33-
if ($row->getAudioPHID()) {
34-
if ($row->getAudioBehavior() != $behavior_none) {
35-
$spec += array(
36-
'audio' => $row->getAudioPHID(),
37-
'audioBehavior' => $row->getAudioBehavior(),
38-
);
39-
}
40-
}
4129

42-
$this->images[$row->getName()] = $spec;
43-
}
30+
$this->macros = mpull($rows, 'getPHID', 'getName');
4431
}
4532

4633
$name = (string)$matches[1];
34+
if (empty($this->macros[$name])) {
35+
return $matches[1];
36+
}
4737

48-
if (array_key_exists($name, $this->images)) {
49-
$phid = $this->images[$name]['image'];
38+
$engine = $this->getEngine();
5039

51-
$file = id(new PhabricatorFile())->loadOneWhere('phid = %s', $phid);
5240

53-
if ($this->getEngine()->isTextMode()) {
41+
$metadata_key = self::KEY_RULE_MACRO;
42+
$metadata = $engine->getTextMetadata($metadata_key, array());
43+
44+
$token = $engine->storeText('<macro>');
45+
$metadata[] = array(
46+
'token' => $token,
47+
'phid' => $this->macros[$name],
48+
'original' => $name,
49+
);
50+
51+
$engine->setTextMetadata($metadata_key, $metadata);
52+
53+
return $token;
54+
}
55+
56+
public function didMarkupText() {
57+
$engine = $this->getEngine();
58+
$metadata_key = self::KEY_RULE_MACRO;
59+
$metadata = $engine->getTextMetadata($metadata_key, array());
60+
61+
if (!$metadata) {
62+
return;
63+
}
64+
65+
$phids = ipull($metadata, 'phid');
66+
$viewer = $this->getEngine()->getConfig('viewer');
67+
68+
// Load all the macros.
69+
$macros = id(new PhabricatorMacroQuery())
70+
->setViewer($viewer)
71+
->withStatus(PhabricatorMacroQuery::STATUS_ACTIVE)
72+
->withPHIDs($phids)
73+
->execute();
74+
$macros = mpull($macros, null, 'getPHID');
75+
76+
// Load all the images and audio.
77+
$file_phids = array_merge(
78+
array_values(mpull($macros, 'getFilePHID')),
79+
array_values(mpull($macros, 'getAudioPHID')));
80+
81+
$file_phids = array_filter($file_phids);
82+
83+
$files = array();
84+
if ($file_phids) {
85+
$files = id(new PhabricatorFileQuery())
86+
->setViewer($viewer)
87+
->withPHIDs($file_phids)
88+
->execute();
89+
$files = mpull($files, null, 'getPHID');
90+
}
91+
92+
// Replace any macros that we couldn't load the macro or image for with
93+
// the original text.
94+
foreach ($metadata as $key => $spec) {
95+
$macro = idx($macros, $spec['phid']);
96+
if ($macro) {
97+
$file = idx($files, $macro->getFilePHID());
5498
if ($file) {
55-
$name .= ' <'.$file->getBestURI().'>';
99+
continue;
56100
}
57-
return $this->getEngine()->storeText($name);
58101
}
59102

103+
$engine->overwriteStoredText($spec['token'], $spec['original']);
104+
unset($metadata[$key]);
105+
}
106+
107+
foreach ($metadata as $spec) {
108+
$macro = $macros[$spec['phid']];
109+
$file = $files[$macro->getFilePHID()];
110+
$src_uri = $file->getBestURI();
111+
112+
if ($this->getEngine()->isTextMode()) {
113+
$result = $spec['original'].' <'.$src_uri.'>';
114+
$engine->overwriteStoredText($spec['token'], $result);
115+
continue;
116+
}
117+
118+
$file_data = $file->getMetadata();
60119
$style = null;
61-
$src_uri = null;
62-
if ($file) {
63-
$src_uri = $file->getBestURI();
64-
$file_data = $file->getMetadata();
65-
$height = idx($file_data, PhabricatorFile::METADATA_IMAGE_HEIGHT);
66-
$width = idx($file_data, PhabricatorFile::METADATA_IMAGE_WIDTH);
67-
if ($height && $width) {
68-
$style = sprintf(
69-
'height: %dpx; width: %dpx;',
70-
$height,
71-
$width);
72-
}
120+
$height = idx($file_data, PhabricatorFile::METADATA_IMAGE_HEIGHT);
121+
$width = idx($file_data, PhabricatorFile::METADATA_IMAGE_WIDTH);
122+
if ($height && $width) {
123+
$style = sprintf(
124+
'height: %dpx; width: %dpx;',
125+
$height,
126+
$width);
73127
}
74128

75129
$id = null;
76-
$audio_phid = idx($this->images[$name], 'audio');
77-
if ($audio_phid) {
130+
$audio = idx($files, $macro->getAudioPHID());
131+
if ($audio) {
78132
$id = celerity_generate_unique_node_id();
79133

80134
$loop = null;
81-
switch (idx($this->images[$name], 'audioBehavior')) {
135+
switch ($macro->getAudioBehavior()) {
82136
case PhabricatorFileImageMacro::AUDIO_BEHAVIOR_LOOP:
83137
$loop = true;
84138
break;
85139
}
86140

87-
$file = id(new PhabricatorFile())->loadOneWhere(
88-
'phid = %s',
89-
$audio_phid);
90-
if ($file) {
91-
Javelin::initBehavior(
92-
'audio-source',
93-
array(
94-
'sourceID' => $id,
95-
'audioURI' => $file->getBestURI(),
96-
'loop' => $loop,
97-
));
98-
}
141+
Javelin::initBehavior(
142+
'audio-source',
143+
array(
144+
'sourceID' => $id,
145+
'audioURI' => $audio->getBestURI(),
146+
'loop' => $loop,
147+
));
99148
}
100149

101-
$img = phutil_tag(
150+
$result = phutil_tag(
102151
'img',
103152
array(
104153
'id' => $id,
105154
'src' => $src_uri,
106-
'alt' => $matches[1],
107-
'title' => $matches[1],
155+
'alt' => $spec['original'],
156+
'title' => $spec['original'],
108157
'style' => $style,
109158
));
110159

111-
return $this->getEngine()->storeText($img);
112-
} else {
113-
return $matches[1];
160+
$engine->overwriteStoredText($spec['token'], $result);
114161
}
162+
163+
$engine->setTextMetadata($metadata_key, array());
115164
}
116165

117166
}

0 commit comments

Comments
 (0)