r108183 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r108182‎ | r108183 | r108184 >
Date:23:25, 5 January 2012
Author:bawolff
Status:ok
Tags:
Comment:
Make sure that if we fail to read the App13 (iptc) block of a JPG file, that that doesn't block other metadata from being read. Also makes sure if more then one app13 block is in the file, they are all read, not just the last one that appears in the file (This required some changes to tests since before the intermediate value was just one value, now its an array of all such blocks)
Modified paths:
  • /trunk/phase3/RELEASE-NOTES-1.19 (modified) (history)
  • /trunk/phase3/includes/media/BitmapMetadataHandler.php (modified) (history)
  • /trunk/phase3/includes/media/JpegMetadataExtractor.php (modified) (history)
  • /trunk/phase3/tests/phpunit/data/media/iptc-invalid-psir.jpg (added) (history)
  • /trunk/phase3/tests/phpunit/includes/media/BitmapMetadataHandlerTest.php (modified) (history)
  • /trunk/phase3/tests/phpunit/includes/media/JpegMetadataExtractorTest.php (modified) (history)

Diff [purge]

Index: trunk/phase3/RELEASE-NOTES-1.19
——@@ -223,6 +223,8 @@
224224 * (bug 33321) Adding a line to MediaWiki:Sidebar that contains a pipe, but doesn't
225225 have any pipes after being transformed by MessageCache, causes exception on
226226 all pages.
 227+* Files with IPTC blocks we can't read no longer prevent extraction of exif
 228+ or other metadata.
227229
228230 === API changes in 1.19 ===
229231 * (bug 19838) siprop=interwikimap can now use the interwiki cache.
Index: trunk/phase3/tests/phpunit/includes/media/BitmapMetadataHandlerTest.php
——@@ -56,6 +56,16 @@
5757 $meta['JPEGFileComment'][0] );
5858 }
5959
 60+ /**
 61+ * Make sure a bad iptc block doesn't stop the other metadata
 62+ * from being extracted.
 63+ */
 64+ public function testBadIPTC() {
 65+ $meta = BitmapMetadataHandler::Jpeg( $this->filePath .
 66+ 'iptc-invalid-psir.jpg' );
 67+ $this->assertEquals( 'Created with GIMP', $meta['JPEGFileComment'][0] );
 68+ }
 69+
6070 public function testIPTCDates() {
6171 $meta = BitmapMetadataHandler::Jpeg( $this->filePath .
6272 'iptc-timetest.jpg' );
Index: trunk/phase3/tests/phpunit/includes/media/JpegMetadataExtractorTest.php
——@@ -59,7 +59,7 @@
6060 public function testPSIRExtraction() {
6161 $res = JpegMetadataExtractor::segmentSplitter( $this->filePath . 'jpeg-xmp-psir.jpg' );
6262 $expected = '50686f746f73686f7020332e30003842494d04040000000000181c02190004746573741c02190003666f6f1c020000020004';
63 - $this->assertEquals( $expected, bin2hex( $res['PSIR'] ) );
 63+ $this->assertEquals( $expected, bin2hex( $res['PSIR'][0] ) );
6464 }
6565 public function testXMPExtractionAltAppId() {
6666 $res = JpegMetadataExtractor::segmentSplitter( $this->filePath . 'jpeg-xmp-alt.jpg' );
——@@ -70,19 +70,19 @@
7171
7272 public function testIPTCHashComparisionNoHash() {
7373 $segments = JpegMetadataExtractor::segmentSplitter( $this->filePath . 'jpeg-xmp-psir.jpg' );
74 - $res = JpegMetadataExtractor::doPSIR( $segments['PSIR'] );
 74+ $res = JpegMetadataExtractor::doPSIR( $segments['PSIR'][0] );
7575
7676 $this->assertEquals( 'iptc-no-hash', $res );
7777 }
7878 public function testIPTCHashComparisionBadHash() {
7979 $segments = JpegMetadataExtractor::segmentSplitter( $this->filePath . 'jpeg-iptc-bad-hash.jpg' );
80 - $res = JpegMetadataExtractor::doPSIR( $segments['PSIR'] );
 80+ $res = JpegMetadataExtractor::doPSIR( $segments['PSIR'][0] );
8181
8282 $this->assertEquals( 'iptc-bad-hash', $res );
8383 }
8484 public function testIPTCHashComparisionGoodHash() {
8585 $segments = JpegMetadataExtractor::segmentSplitter( $this->filePath . 'jpeg-iptc-good-hash.jpg' );
86 - $res = JpegMetadataExtractor::doPSIR( $segments['PSIR'] );
 86+ $res = JpegMetadataExtractor::doPSIR( $segments['PSIR'][0] );
8787
8888 $this->assertEquals( 'iptc-good-hash', $res );
8989 }
Index: trunk/phase3/tests/phpunit/data/media/iptc-invalid-psir.jpg
Cannot display: file marked as a binary type.
svn:mime-type = image/jpeg
Property changes on: trunk/phase3/tests/phpunit/data/media/iptc-invalid-psir.jpg
___________________________________________________________________
Added: svn:mime-type
9090 + image/jpeg
Index: trunk/phase3/includes/media/BitmapMetadataHandler.php
——@@ -32,7 +32,15 @@
3333 * @param String $app13 String containing app13 block from jpeg file
3434 */
3535 private function doApp13 ( $app13 ) {
36 - $this->iptcType = JpegMetadataExtractor::doPSIR( $app13 );
 36+ try {
 37+ $this->iptcType = JpegMetadataExtractor::doPSIR( $app13 );
 38+ } catch ( MWException $e ) {
 39+ // Error reading the iptc hash information.
 40+ // This probably means the App13 segment is something other than what we expect.
 41+ // However, still try to read it, and treat it as if the hash didn't exist.
 42+ wfDebug( "Error parsing iptc data of file: " . $e->getMessage() . "\n" );
 43+ $this->iptcType = 'iptc-no-hash';
 44+ }
3745
3846 $iptc = IPTC::parse( $app13 );
3947 $this->addMetadata( $iptc, $this->iptcType );
——@@ -122,8 +130,10 @@
123131 if ( isset( $seg['COM'] ) && isset( $seg['COM'][0] ) ) {
124132 $meta->addMetadata( Array( 'JPEGFileComment' => $seg['COM'] ), 'native' );
125133 }
126 - if ( isset( $seg['PSIR'] ) ) {
127 - $meta->doApp13( $seg['PSIR'] );
 134+ if ( isset( $seg['PSIR'] ) && count( $seg['PSIR'] ) > 0 ) {
 135+ foreach( $seg['PSIR'] as $curPSIRValue ) {
 136+ $meta->doApp13( $curPSIRValue );
 137+ }
128138 }
129139 if ( isset( $seg['XMP'] ) && $showXMP ) {
130140 $xmp = new XMPReader();
Index: trunk/phase3/includes/media/JpegMetadataExtractor.php
——@@ -31,6 +31,7 @@
3232 $segments = array(
3333 'XMP_ext' => array(),
3434 'COM' => array(),
 35+ 'PSIR' => array(),
3536 );
3637
3738 if ( !$filename ) {
——@@ -122,7 +123,7 @@
123124 // APP13 - PSIR. IPTC and some photoshop stuff
124125 $temp = self::jpegExtractMarker( $fh );
125126 if ( substr( $temp, 0, 14 ) === "Photoshop 3.0\x00" ) {
126 - $segments["PSIR"] = $temp;
 127+ $segments["PSIR"][] = $temp;
127128 }
128129 } elseif ( $buffer === "\xD9" || $buffer === "\xDA" ) {
129130 // EOI - end of image or SOS - start of scan. either way we're past any interesting segments
——@@ -162,11 +163,12 @@
163164 * This should generally be called by BitmapMetadataHandler::doApp13()
164165 *
165166 * @param String $app13 photoshop psir app13 block from jpg.
 167+ * @throws MWException (It gets caught next level up though)
166168 * @return String if the iptc hash is good or not.
167169 */
168170 public static function doPSIR ( $app13 ) {
169171 if ( !$app13 ) {
170 - return;
 172+ throw new MWException( "No App13 segment given" );
171173 }
172174 // First compare hash with real thing
173175 // 0x404 contains IPTC, 0x425 has hash
——@@ -218,8 +220,8 @@
219221
220222 // this should not happen, but check.
221223 if ( $lenData['len'] + $offset > $appLen ) {
222 - wfDebug( __METHOD__ . " PSIR data too long.\n" );
223 - return 'iptc-no-hash';
 224+ throw new MWException( "PSIR data too long. (item length=" . $lenData['len']
 225+ . "; offset=$offset; total length=$appLen)" );
224226 }
225227
226228 if ( $valid ) {

Status & tagging log