Discussion:
[gs-bugs] [Bug 698246] - MuPDF - Mistakes in mupdf_explored
b***@artifex.com
2017-07-13 17:31:58 UTC
Permalink
http://bugs.ghostscript.com/show_bug.cgi?id=698246

Bug ID: 698246
Summary: Mistakes in mupdf_explored
Product: MuPDF
Version: master
Hardware: PC
OS: Windows NT
Status: UNCONFIRMED
Severity: normal
Priority: P4
Component: documentation
Assignee: ***@artifex.com
Reporter: ***@artifex.com
QA Contact: gs-***@ghostscript.com
Word Size: ---

Bug to hold issues discovered in the text of MuPDF Explored.
--
You are receiving this mail because:
You are the QA Contact for the bug.
b***@artifex.com
2017-07-13 17:37:27 UTC
Permalink
http://bugs.ghostscript.com/show_bug.cgi?id=698246

***@gmail.com changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@gmail.com

--- Comment #1 from ***@gmail.com ---
FZ_IGNORE_[SHADE|IMAGE] is mentioned several times, those enum values were
removed.
--
You are receiving this mail because:
You are the QA Contact for the bug.
b***@artifex.com
2017-07-13 17:38:12 UTC
Permalink
http://bugs.ghostscript.com/show_bug.cgi?id=698246

--- Comment #2 from ***@gmail.com ---
page 53 FZ_DONT_INTERPOLAGE_IMAGES - typo
--
You are receiving this mail because:
You are the QA Contact for the bug.
b***@artifex.com
2017-07-13 17:43:35 UTC
Permalink
http://bugs.ghostscript.com/show_bug.cgi?id=698246

--- Comment #3 from ***@gmail.com ---
Page 56 - For more information about page manipulation in PDF documents, see ??
??.
--
You are receiving this mail because:
You are the QA Contact for the bug.
b***@artifex.com
2017-07-13 17:49:27 UTC
Permalink
http://bugs.ghostscript.com/show_bug.cgi?id=698246

--- Comment #4 from ***@gmail.com ---
"8.2.1Basic Colorspaces
MuPDF contains a set of inbuilt colorspaces that cover most simple require-
ments. These can

8.3"
--
You are receiving this mail because:
You are the QA Contact for the bug.
b***@artifex.com
2017-07-13 20:21:10 UTC
Permalink
http://bugs.ghostscript.com/show_bug.cgi?id=698246

--- Comment #5 from ***@gmail.com ---
page 98 - "Display lists are implemented within using MuPDF using the fz
display list type."
--
You are receiving this mail because:
You are the QA Contact for the bug.
b***@artifex.com
2017-07-13 20:28:39 UTC
Permalink
http://bugs.ghostscript.com/show_bug.cgi?id=698246

--- Comment #6 from ***@gmail.com ---
There are "(see /rjwrefXXX)" in many places (for instance page 121)
--
You are receiving this mail because:
You are the QA Contact for the bug.
b***@artifex.com
2017-08-05 12:24:18 UTC
Permalink
http://bugs.ghostscript.com/show_bug.cgi?id=698246

Sebastian Rasmussen <***@hotmail.com> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |***@hotmail.com

--- Comment #7 from Sebastian Rasmussen <***@hotmail.com> ---
A few suggestsions:

1.2 License:

Page 2: "you *must* abide by the following terms." --> "you *must* abide by the
terms described by GNU AGPL. These are explained in layman's language below,
but the legal requirements are still those set out in GNU AGPL."

Page 2: "you must license the entirety of that installation under the GNU AGPL"
--> "you must license the3 entirety of that installation under the GNU AGPL or
a compatible license
(https://www.gnu.org/licenses/license-list.en.html#GPLCompatibleLicenses)."

Page 2: "must be licensed under the GNU AGPL." --> "must be licensed under the
GNU AGPL or a compatible license
(https://www.gnu.org/licenses/license-list.en.html#GPLCompatibleLicenses)."

Page 2: Perhaps also add a section along the lines of
"Because of Artifex makes MuPDF available under both GNU AGPL and a commercial
license contributing bug fixes or patches to the project requires a signed
Contributors License Agreement (permanent(!) link to artifex.com where the CLA
is available). This is required in order for Artifex to retain the copyright of
the full source code, so as to be able to relicense it under a commercial
license for its customers."

1.3 Dependencies:

Page 3: Mention LCMS2 now that is has been integrated.

Page 3: "OpenType Font shaper built upon freetype, required for e-pub files."
--> "OpenType Font shaper required for e-pub files, handles RTL languages." or
something to that effect

Page 3: Perhaps mention the other J2K image decoding library? And why it might
be better? And also when it is available?

3.4 Multi-threading

Page 9: "multi threaded" --> "multi-threaded"

Page 9: "MuPDF guarantees never to take lock n if that thread already holds
lock m (for n ¿ m )." The upside-down question mark is likely not the character
meant to be here...

Page 9: Perhaps add something to the effect of:
"Each displaylist can safely be used from several different threads as long as
there are safeguards in place to prevent the usages being simultaneous."

3.7 Tuning

Page 11: Why would I want to decode a bigger subarea of an image when only a
smaller one is required? What happens if a decode a smaller subarea? Would
images be correctly decoded but at a lower sample rate?

Page 11: A short explanation of what Mitchell scaling is? What are the trade
offs when using/not using it?

3.8 Summary

Page 13: Maybe add "Display lists can only be used in 1 thread at a time."?

4.1 Overview

Page 14: Describe that it is not ok to call functions that may throw in
fz_always() (or else all the code in fz_always() will not be executed). This
also makes it vital to explain somewhere that fz_free() and fz_drop*() and
pdf_drop*() may not throw exceptions, making them safe to call from
fz_always().

Page 15: "If you do not wish to understand the long and complex reasons behind
this, skip forward to the summary and just follow the rules there." If I do
indeed skip until the Summary, this part nor Summary explains what protecting a
variable by a fz_var() call looks like. I suggest putting an example here and
the cited sentence above after the example..?

Page 15: "fz_try/fz/catch()" has a slash typo. "As stated before fz try/ fz
catch" has a space too much after the slash (this happens on page 16 too, is
this stylistic? If so, why does setjmp/longjmp not have a space after the
slash?)

Page 15: Perhaps put emphasis on that if and only if fz_rethrow() is called
will the exception be propagated to the enclosing block? And conversely
explains that execution will simply continue after fz_catch() unless
fz_rethrow() is called.

Page 16: "Or fz_rethrow if we’re nested." what does this comment mean? Maybe it
is better to explain what happens if you call NULL: "Do not propagate the
exception, silently return NULL instead." I would get it instantly. It might be
better to clearly explain fz_rethrow() in the previous example as I try to
suggest above. :)

Page 16: Maybe explain that the combined house has to keep its own references
to the walls and the roof and that what we drop in fz_always() are the local
references not owned by the house?

Page 16: "this means that any local variables set within the fz_try block can
be 'lost'" --> "this means that any local variables set within the fz_try block
can have their new values 'lost'" or perhaps "this means that any changes to
local variables within the fz_try block can be 'lost'"

Page 16: "By calling fz var(w);" --> "By calling fz var(w) before fz_try()"

Page 16: "The 'loss' of the value occurs because the compiler can postpone
writing the value back into the storage location for the variable (or can
choose to just hold it in a register)." --> "The 'loss' of the value occurs
because the compiler can keep a temporary copy of a value in a register,
postponing writing the value back into the storage location for the variable
(or can choose to optimize the code to always hold the value in a register)."

page 17: longjmp should be bold..?

Page 17: "any functions it call" --> "any functions it calls"

Page 17: "Hence the variable is magically protected." --> "Hence the variable
is guaranteed to have the last value set, regardless of whether longjmp is
called or not."

4.2 Throwing exceptions

Page 18: What are the non-FZ_ERROR_GENERIC values used for? Are there some
values that should only be used by the core library?

4.3 Handling exceptions

Page 18: Explain that fz_caught() should only be inside fz_catch or be called
if and only if fz_catch has been executed (while an exception is still on the
exception stack). Also how does looking at the error code help in retrying the
same code in a different manner? Isn't it better to state that an application
might want to retry executing upon some type of exceptions, while erroring out
on other types..?

Page 18: "convenience function that with rethrow just a particular type.:" -->
"convenience function that will rethrow just for a particular type:" Notice the
removed period.

4.4 Summary

Page 19: "within an fz_try" --> "within a fz_try" as long as we read that as
"within a fitz try", probably several instances of "an fz_"... But if you read
that as "within an EFF ZED try" then of course this comment is no longer valid.
:)
--
You are receiving this mail because:
You are the QA Contact for the bug.
b***@artifex.com
2017-08-05 13:33:24 UTC
Permalink
http://bugs.ghostscript.com/show_bug.cgi?id=698246

--- Comment #8 from Sebastian Rasmussen <***@hotmail.com> ---
5.1 Overview

Page 21: "If not the code forms the data itself, and then puts it into the
store." --> "If not the code decodes the data, and then puts it into the
store." since you described this process as decoding a paragraphs above.

Page 21: "objects are evicted from the store" --> "objects are evicted from the
store (incuring a slight performance degradation when the objects are referred
to in the future)"..?

Page 21: "should be treated exactly as for normal objects and kept/dropped as
usual." --> "should be treated exactly as normal objects and kept/dropped as
usual."

Page 21: Should it be mentioned that if fz_new*() fails that function must
clean up any objects it might have created halfway and that fz_drop*() and
fz_keep*() never throw..?

Page 22: "<code to free the contents of the path structure>" --> "/* code to
free the contents of the path structure */" just like in other places where C
comments are used?

Page 22: Maybe mention that it is up to the developer to guarantee that the
number of references always be under the implementation limit of int8_t (127)
and int16_t (32767) as fz_keep() will not handle or warn in this case.

5.3 Creating the Store

Page 22: "FZ_STORE_UNLIMITED (0)" --> "FZ_STORE_UNLIMITED" is its value is not
important and avoiding the reader conflating it with a function.

5.4 Using the store
This heading should be using "Store" to be consistent

5.4.1 Overview

Page 23: "the fact that the object has been used is noted", what does this
mean? Is this the same as taking a reference? If so the next clause "a
reference is taken" covers that..?

Page 23 "This ensures that the Store's figure for 'amount of memory used by
fz_storable's' remains correct (thus ensuring that should objects become
evictable, the store size will fall correctly)." I'm not sure what this means,
though I'm guessing this part refers to how the code previously incorrectly
handled the case of being unable to evict objects from the store..? Does that
need to be explained now that it works correctly?

Page 23: "It also does no harm, because clearly we have managed to allocate
enough memory to form the fz_storable in the first place." --> "It also does no
harm, because clearly we have managed to allocate enough memory to decode the
fz_storable in the first place." as we replaced form with decode before for
consistency.

Page 23: Maybe also explain that all objects in the store are evicted as part
of dropping the last reference to the context..? I.e. there is no need to
trying to empty the store yourself.

5.4.2 Handling keys

Page 24: "the keep and drop functions provided here can take and drop these in
sync." what does in sync mean..?

Page 24: "hashtable" or "hash table"?

5.4.4 Key storeable items

Page 25: "We refer to these objects as “key storable”" but other places use
single quotes instead of double quotes.

Page 26: "Store for ‘dead’ entries - those that can never be ‘found’ by looking
in the store." Are these dead entries the objects that objects kept in the
Store only because other objects keys refer to them as mentioned before? Might
be good to use the same explanation in that case.

5.4.5 Reap passes

Page 26: "to bracket a region in which many may be triggered." --> "to bracket
a region in which many reapable objects may be dropped at the same time."
Though it might be worth to also explain when reaping happens automatically.
Does an implementer need to trigger the reaping process..?

5.5 Scavenging memory allocator

Page 26: "All allocations within MuPDF (and its sub-libraries) call fz malloc
and family." Not Harfbuzz's globals though! :) Oh, and
freetype/jbig2/libjpeg/zlib/lcms2 appear to refer to malloc() and free() too..?

5.6 Reacting to Out of Memory events

Page 27: FZ_ERROR_OOM is not FZ_ERROR_MEMORY.
--
You are receiving this mail because:
You are the QA Contact for the bug.
b***@artifex.com
2017-12-02 14:08:52 UTC
Permalink
This post might be inappropriate. Click to display it.
b***@artifex.com
2017-12-02 15:23:24 UTC
Permalink
http://bugs.ghostscript.com/show_bug.cgi?id=698246

--- Comment #10 from Sebastian Rasmussen <***@hotmail.com> ---
8.6 Images

Page 58: "To minimise the impact of such decodes, fz_image make use of the
Store" does this take the subarea, trans matrix, etc into account and when it
stores the decoded image into the Store? Do we need to mention it?

8.8 Transforms

Page 62: "the fact that the image of a rectangle may be flipped:" What does
this mean?

8.9 Paths

Page 66: "Postscript (or PDF)" might be better as "PDF (and Postscript)"? The
rest of the paragraph lists the two formats in that order.

Page 69: "we can close the path" Do we need to explain that this might draw a
line to the starting point?

8.9.6 Stroking

page 74: "When filling a path simply requires details of the fill to be used,
stroking a path can radically alter its appereance". Really? I'm not sure why
there is such a major empahsis put on this difference? Perhaps an explanation
is missing here?

Page 77: fz_unshare_stroke_state_with_dash_len() what happens if the dash len
changes in this interface?

8.9.7 Walking

Page 78: "and call a for each 'segment' of the path in turn." The word order
seems a bit strange here.

8.10.1 Overview

Page 79: Perhaps move the overview text directly under 8.10 Text? This has been
done elsewhere.

Page 79: "transmute" -> "translate"? Also what does "logical order" mean
further down the page?

8.10.4 Measurement

Page 82: I like this explanation of bounding, perhaps this can be reused in
other areas where bounding is mentioned?

8.10.7 Implementation

Page 84: "drawing glyphs non-monotonically onto the page" What does
"non-monotonically" mean in this context?

8.11.2 Creation

Page 86: "desirable for the JNI bindings" Possibly remove "the"?

10.3.3 Reading bits

Page 100: There is a stray semicolon after the fz_read_rbits() code sample

Page 100: We might want to explain if fz_sync_bits() skips ahead or rewinds to
the next/previous byte boundary.

10.3.5 Seeking

Page 101: What does fz_tell() return if we read bits?

10.3.6 Meta data

Page 102: "The fz_stream_meta function allows such calls to be made, with a
reason code to identify the required operation" This seems to be called "key"
in the cited code, maybe "request key" or something like that might be better?

11.4 Implementing a fz_output

Page 111: "If !state? needs no destruction"... "!state?" just looks weird...
Perhaps it is worth mentioning that the close function must be called to ensure
that the entire file is written. Perhaps this should also be mentioned when
calling the band writer and document writers..?

12.1 Overview

Page 113: "(see /rjwrefDrawDevice)" needs to be a proper reference?

12.2 Band Writers

Page 114: Is this the same as on page 55?

Page 115: "Once enough data has been sent, the band wrinter automatically
writes any trailer for the file." There is a fz_write_trailer() interface. Does
this have to be called? When? Why?

12.8 PWG/CUPS

Page 118: "and those that take f_bitmaps" this should be "fz_bitmap"..?

12.10.2 Mono

Page 124: "Postscript reuqires file level headers and trailers, over and above
that produced by the band writer itself." So these are not written by the band
writer automatically? Why not?

13 The Image interface

Page 126: Same as page 58? Can it be combined into one?

13.2.3 Display List

Page 130: "The final standard sort of image in MuPDF is that based upon a
display list." Why is this mentioned as an image? That seems contradictory.

Page 130: "we use this to easily embed" Initial capital.

Page 130: "epub" should be "ePUB", right? Maybe search through the entire
document for more typos like these.

13.4 Implementing an Image Type

Page 132: Explain why this would be needed?
--
You are receiving this mail because:
You are the QA Contact for the bug.
b***@artifex.com
2017-12-02 15:53:32 UTC
Permalink
http://bugs.ghostscript.com/show_bug.cgi?id=698246

--- Comment #11 from Sebastian Rasmussen <***@hotmail.com> ---
14.3.1 PDF

Page 146: "i.e up to and including PDF 1.7" I think "i.e." is better and maybe
we should write PDF 2.0?

15.2 Implementation

Page 151: The fz_new_derived_writer() code sample runs out into the margin and
out of the page.

16.2.2 rough renderings

Page 155: "the linearization procedure as laid down by Adobe does NOT do this"
Does this sentence imply that document MAY place all used objects before a page
object or that it CANNOT?

25.5.2 GLFW version

Maybe this should be freeglut now? :)
--
You are receiving this mail because:
You are the QA Contact for the bug.
b***@artifex.com
2017-12-05 17:31:36 UTC
Permalink
http://bugs.ghostscript.com/show_bug.cgi?id=698246

--- Comment #12 from Robin Watts <***@artifex.com> ---
I've run through these and addressed as many as I agree with in the version at
http://ghostscript.com/~robin/mupdf_explored.pdf as of 5/12/2017.

Thanks!
--
You are receiving this mail because:
You are the QA Contact for the bug.
Loading...