'RE: [Filesystem-dev] Re: [Android-port-dev] CR: Fix Bug 13089:.mkv'

[prev in list] [next in list] [prev in thread] [next in thread] 

List: helix-filesystem-dev
Subject: RE: [Filesystem-dev] Re: [Android-port-dev] CR: Fix Bug 13089:.mkv
From: Kinson Liu <kliu () real ! com>
Date: 2011-04-28 7:57:24
Message-ID: DAF5D4199FBBD44489E7240FA175147A087F16BEDF () SEAMBX ! corp ! real ! com
[Download RAW message or body]

Just for everyone's sake, only the last change at the bottom of the diff is=
 what we meant to change.  The rest changes are irrelevant and will not be =
checked in.

Kinson

________________________________
From: filesystem-dev-bounces@helixcommunity.org [mailto:filesystem-dev-boun=
ces@helixcommunity.org] On Behalf Of Dinel Lin
Sent: Thursday, April 28, 2011 8:59 AM
To: Sheldon Fu
Cc: filesystem-dev@helixcommunity.org
Subject: [Filesystem-dev] Re: [Android-port-dev] CR: Fix Bug 13089:.mkv can=
 not be displayed in playlist

OK.Got it.
I didn't consider much more except only my scenario and I agree with your v=
iewpoints.When reading files, we'd better prevent from reading beyond the e=
nd of the file.However, it is impossible to detect the action(reading or wr=
iting) which caller want to perform.As result, caller should be responsible=
 for the seek operation. It

On 04/27/2011 09:35 PM, Sheldon Fu wrote:
Also this CR should go to filesystem-dev since it won't be an Android speci=
fic change.

fxd

On 04/27/2011 09:34 AM, Sheldon Fu wrote:
Seeking can be from current position (SEEK_CUR) so it is not correct to alw=
ays compare the offset to file size. Also in the file writing case, you can=
 actually seek beyond current file size and write -- the API will fill the =
gap for you. Because of this, seeking beyond file size can not generally be=
 considered a failure case.

We could just take the size check out I think. If the actual file seek fail=
s for any reason, we would detect it anyway.

fxd

On 04/27/2011 08:15 AM, Dinel Lin wrote:
Now, I correct the drawback by comparing the offset with file size.
It seems more helpful.

On 04/27/2011 07:54 PM, Sheldon Fu wrote:
If what you need is to allow seek to a larger offset, not to read a larger =
chunk of data, then you should fix the seek logic.

IMHO, we should just take out the seek limit. It is pointless to limit the =
seek range anyway (even worse to limit it to MAX_READ_SIZE as the code is n=
ow).

fxd

On 04/27/2011 07:47 AM, Dinel Lin wrote:
Yes, sheldon.
Actually, it will impact the heap using and it was my fault not adding it.H=
owever, in some kind of scenario, we need to seek with a quite large positi=
on(larger that current setting 256M).So enlarge this limitation can help us=
 to seek successfully. As your said, 4G seems quite large.I didn't find a m=
ore appropriate value but only reference to 361 branch.
thanks

On 04/27/2011 07:35 PM, Sheldon Fu wrote:
That's 4G bytes! You are essentially taking out the read size limitation.  =
Are we sure this has no 'Heap Use impact'?

The old value is 256M, which is rather large for a single read action alrea=
dy. FF plugin is not supposed to issue read request with such a large size =
-- it's not good for memory usage. We should look into a fix in the FF itse=
lf I think.

fxd

On 04/27/2011 07:00 AM, Dinel Lin wrote:
Modified by: zlin@real.com<mailto:zlin@real.com>
Date: 04/27/2011
Project: RealPlayer for Android Smartphones(LePhone-TD)

Synopsis:specific mkv file can not be displayed in playlist

Overview:Some large mkv file can not be listed in the video list. It is due=
 to seek to specific file position failed, then get into a infinite loop.So=
, in order to seek large file successfully, enlarge the MAX_READ_SIZE to fu=
lfill the requirements(Thanks to joe for giving me this advice).Meanwhile, =
update file system object source file according to 361 branch.

Files Added:
none

Files Modified:
filesystem/local/full/smplfsys.cpp

Image Size and Heap Use impact (Client -Only):
None

Platforms and Profiles Affected:
Platform: android-2.3-arm-smdk_c110
Profile: helix-client-android-full

Distribution Libraries Affected:
None

Distribution library impact and planned action:
None

Platforms and Profiles Build Verified:
Platform: android-2.3-arm-smdk_c110
Profile: helix-client-android-full

Platforms and Profiles Functionality verified:
Platform: android-2.3-arm-smdk_c110
Profile: helix-client-android-full

Branch: 365atlas

Best regards
zlin

[Attachment #3 (text/html)]

<html xmlns:v="urn:schemas-microsoft-com:vml" \
xmlns:o="urn:schemas-microsoft-com:office:office" \
xmlns:w="urn:schemas-microsoft-com:office:word" \
xmlns:st1="urn:schemas-microsoft-com:office:smarttags" \
xmlns="http://www.w3.org/TR/REC-html40">

<head>
<META HTTP-EQUIV="Content-Type" CONTENT="text/html; charset=us-ascii">
<meta name=Generator content="Microsoft Word 11 (filtered medium)">
<!--[if !mso]>
<style>
v\:* {behavior:url(#default#VML);}
o\:* {behavior:url(#default#VML);}
w\:* {behavior:url(#default#VML);}
.shape {behavior:url(#default#VML);}
</style>
<![endif]--><o:SmartTagType
 namespaceuri="urn:schemas-microsoft-com:office:smarttags" name="chsdate"/>
<o:SmartTagType namespaceuri="urn:schemas-microsoft-com:office:smarttags"
 name="chmetcnv"/>
<!--[if !mso]>
<style>
st1\:*{behavior:url(#default#ieooui) }
</style>
<![endif]-->
<style>
<!--
 /* Font Definitions */
 @font-face
	{font-family:SimSun;
	panose-1:2 1 6 0 3 1 1 1 1 1;}
@font-face
	{font-family:SimSun;
	panose-1:2 1 6 0 3 1 1 1 1 1;}
@font-face
	{font-family:Tahoma;
	panose-1:2 11 6 4 3 5 4 4 2 4;}
 /* Style Definitions */
 p.MsoNormal, li.MsoNormal, div.MsoNormal
	{margin:0cm;
	margin-bottom:.0001pt;
	font-size:12.0pt;
	font-family:"Times New Roman";
	color:black;}
a:link, span.MsoHyperlink
	{color:blue;
	text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
	{color:blue;
	text-decoration:underline;}
span.EmailStyle18
	{mso-style-type:personal-reply;
	font-family:Arial;
	color:navy;}
@page Section1
	{size:595.3pt 841.9pt;
	margin:72.0pt 90.0pt 72.0pt 90.0pt;}
div.Section1
	{page:Section1;}
-->
</style>
<!--[if gte mso 9]><xml>
 <o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
 <o:shapelayout v:ext="edit">
  <o:idmap v:ext="edit" data="1" />
 </o:shapelayout></xml><![endif]-->
</head>

<body bgcolor=white lang=ZH-CN link=blue vlink=blue>

<div class=Section1>

Just for everyone&#8217;s
sake, only the last change at the bottom of the diff is what we meant to
change. &nbsp;The rest changes are irrelevant and will not be checked \
in.<o:p></o:p>

<p class=MsoNormal><font size=1 color=navy face=Arial><span lang=EN-US
style='font-size:9.0pt;font-family:Arial;color:navy'><o:p>&nbsp;</o:p></span></font></p>

<p class=MsoNormal><font size=1 color=navy face=Arial><span lang=EN-US
style='font-size:9.0pt;font-family:Arial;color:navy'>Kinson<o:p></o:p></span></font></p>

<p class=MsoNormal><font size=1 color=navy face=Arial><span lang=EN-US
style='font-size:9.0pt;font-family:Arial;color:navy'><o:p>&nbsp;</o:p></span></font></p>

<div>

<div class=MsoNormal align=center style='text-align:center'><font size=3
color=black face="Times New Roman"><span lang=EN-US style='font-size:12.0pt;
color:windowtext'>

<hr size=2 width="100%" align=center tabindex=-1>

</span></font></div>

<p class=MsoNormal><b><font size=2 color=black face=Tahoma><span lang=EN-US
style='font-size:10.0pt;font-family:Tahoma;color:windowtext;font-weight:bold'>From:</span></font></b><font
 size=2 color=black face=Tahoma><span lang=EN-US style='font-size:10.0pt;
font-family:Tahoma;color:windowtext'> filesystem-dev-bounces@helixcommunity.org
[mailto:filesystem-dev-bounces@helixcommunity.org] <b><span style='font-weight:
bold'>On Behalf Of </span></b>Dinel Lin<br>
<b><span style='font-weight:bold'>Sent:</span></b> Thursday, April 28, 2011
8:59 AM<br>
<b><span style='font-weight:bold'>To:</span></b> Sheldon Fu<br>
<b><span style='font-weight:bold'>Cc:</span></b> \
filesystem-dev@helixcommunity.org<br> <b><span \
style='font-weight:bold'>Subject:</span></b> [Filesystem-dev] Re: [Android-port-dev] \
CR: Fix Bug 13089:.mkv can not be displayed in playlist</span></font><font \
color=black><span lang=EN-US style='color:windowtext'><o:p></o:p></span></font></p>

</div>

<p class=MsoNormal><font size=3 color=black face="Times New Roman"><span
lang=EN-US style='font-size:12.0pt'><o:p>&nbsp;</o:p></span></font></p>

OK.Got it. 
I didn't consider much more except only my scenario and I agree with your
viewpoints.When reading files, we'd better prevent from reading beyond the end
of the file.However, it is impossible to detect the action(reading or writing)
which caller want to perform.As result, caller should be responsible for the
seek operation. It 
 
On 04/27/2011 09:35 PM, Sheldon Fu wrote: <o:p></o:p>

<p class=MsoNormal><font size=3 color=black face="Times New Roman"><span
lang=EN-US style='font-size:12.0pt'>Also this CR should go to filesystem-dev
since it won't be an Android specific change.<br>
<br>
fxd<br>
<br>
On 04/27/2011 09:34 AM, Sheldon Fu wrote: <o:p></o:p></span></font></p>

Seeking can be from current position
(SEEK_CUR) so it is not correct to always compare the offset to file size. Also
in the file writing case, you can actually seek beyond current file size and
write -- the API will fill the gap for you. Because of this, seeking beyond
file size can not generally be considered a failure case. 
 
We could just take the size check out I think. If the actual file seek fails
for any reason, we would detect it anyway. 
 
fxd 
 
On 04/27/2011 08:15 AM, Dinel Lin wrote: <o:p></o:p>

<p class=MsoNormal><font size=3 color=black face="Times New Roman"><span
lang=EN-US style='font-size:12.0pt'>Now, I correct the drawback by comparing
the offset with file size.<br>
It seems more helpful.<br>
<br>
<br>
On 04/27/2011 07:54 PM, Sheldon Fu wrote: <o:p></o:p></span></font></p>

If what you need is to allow seek to a
larger offset, not to read a larger chunk of data, then you should fix the seek
logic. 
 
IMHO, we should just take out the seek limit. It is pointless to limit the seek
range anyway (even worse to limit it to MAX_READ_SIZE as the code is now). 
 
fxd 
 
On 04/27/2011 07:47 AM, Dinel Lin wrote: <o:p></o:p>

Yes, sheldon. 
Actually, it will impact the heap using and it was my fault not adding
it.However, in some kind of scenario, we need to seek with a quite large
position(larger that current setting <st1:chmetcnv TCSC="0" NumberType="1"
Negative="False" HasSpace="False" SourceValue="256" UnitName="m" \
w:st="on">256M</st1:chmetcnv>).So enlarge this limitation can help us to seek \
successfully. As your said, <st1:chmetcnv TCSC="0" NumberType="1" Negative="False" \
HasSpace="False" SourceValue="4" UnitName="g" w:st="on">4G</st1:chmetcnv> seems quite \
large.I didn't find a more appropriate value but only reference to 361 branch. 
thanks 
 
 
On 04/27/2011 07:35 PM, Sheldon Fu wrote: <o:p></o:p>

<p class=MsoNormal><font size=3 color=black face="Times New Roman"><span
lang=EN-US style='font-size:12.0pt'>That's <st1:chmetcnv TCSC="0" NumberType="1"
Negative="False" HasSpace="False" SourceValue="4" UnitName="g" \
w:st="on">4G</st1:chmetcnv> bytes! You are essentially taking out the read size \
limitation.&nbsp; Are we sure this has no '</span></font><span \
class=apple-style-span><font size=2 face="Courier New"><span lang=EN-US \
style='font-size:10.0pt;font-family:"Courier New"'><span style='orphans: 2;widows: \
2;word-spacing:0px'>Heap Use impact'?</span></font></span><font size=2 face="Courier \
New"><span lang=EN-US style='font-size:10.0pt;font-family: "Courier New"'><br>
<br>
<span class=apple-style-span>The old value is <st1:chmetcnv TCSC="0"
NumberType="1" Negative="False" HasSpace="False" SourceValue="256" UnitName="m"
w:st="on">256M</st1:chmetcnv>, which is rather large for a single read action
already. FF plugin is not supposed to issue read request with such a large size
-- it's not good for memory usage. We should look into a fix in the FF itself I
think.</span><br>
<br>
<span class=apple-style-span>fxd</span><br>
</span></font><span lang=EN-US></span><br>
On 04/27/2011 07:00 AM, Dinel Lin wrote: <o:p></o:p></span></p>

<p class=MsoNormal><span class=apple-style-span><font size=2 color=black
face="Courier New"><span lang=EN-US style='font-size:10.0pt;font-family:"Courier \
New"'><span style='orphans: 2;widows: 2;word-spacing:0px'>Modified by: <a
href="mailto:zlin@real.com" \
moz-do-not-send=true>zlin@real.com</a></span></font></span><font size=2 face="Courier \
New"><span lang=EN-US style='font-size:10.0pt;font-family: "Courier New"'><br>
<span class=apple-style-span>Date: <st1:chsdate IsROCDate="False"
IsLunarDate="False" Day="27" Month="4" Year="2011" w:st="on">04/27/2011<br>
</st1:chsdate>Project: RealPlayer for Android Smartphones(LePhone-TD)</span><br>
<br>
<span class=apple-style-span>Synopsis:specific mkv file can not be displayed in
playlist</span><br>
<br>
<span class=apple-style-span>Overview:Some large mkv file can not be listed in
the video list. It is due to seek to specific file position failed, then get
into a infinite loop.So, in order to seek large file successfully, enlarge the
MAX_READ_SIZE to fulfill the requirements(Thanks to joe for giving me this
advice).Meanwhile, update file system object source file according to 361
branch.</span><br>
<br>
<span class=apple-style-span>Files Added:</span><br>
<span class=apple-style-span>none</span><br>
<br>
<span class=apple-style-span>Files Modified:</span><br>
<span class=apple-style-span>filesystem/local/full/smplfsys.cpp</span><br>
<br>
<span class=apple-style-span>Image Size and Heap Use impact (Client \
-Only):</span><br> <span class=apple-style-span>None</span><br>
<br>
<span class=apple-style-span>Platforms and Profiles Affected:</span><br>
<span class=apple-style-span>Platform: android-2.3-arm-smdk_c110</span><br>
<span class=apple-style-span>Profile: helix-client-android-full</span><br>
<br>
<span class=apple-style-span>Distribution Libraries Affected:</span><br>
<span class=apple-style-span>None</span><br>
<br>
<span class=apple-style-span>Distribution library impact and planned \
action:</span><br> <span class=apple-style-span>None</span><br>
<br>
<span class=apple-style-span>Platforms and Profiles Build Verified:</span><br>
<span class=apple-style-span>Platform: android-2.3-arm-smdk_c110</span><br>
<span class=apple-style-span>Profile: helix-client-android-full</span><br>
<br>
<span class=apple-style-span>Platforms and Profiles Functionality \
verified:</span><br> <span class=apple-style-span>Platform: \
android-2.3-arm-smdk_c110</span><br> <span class=apple-style-span>Profile: \
helix-client-android-full</span><br> <br>
<span class=apple-style-span>Branch: 365atlas</span><br>
<br>
<span class=apple-style-span>Best regards</span><br>
<span class=apple-style-span>zlin</span></span></span></font><span lang=EN-US> \
<o:p></o:p></span></p>

<p class=MsoNormal><font size=3 color=black face="Times New Roman"><span
lang=EN-US style='font-size:12.0pt'><o:p>&nbsp;</o:p></span></font></p>

<p class=MsoNormal><font size=3 color=black face="Times New Roman"><span
lang=EN-US style='font-size:12.0pt'><o:p>&nbsp;</o:p></span></font></p>

<p class=MsoNormal><font size=3 color=black face="Times New Roman"><span
lang=EN-US style='font-size:12.0pt'><o:p>&nbsp;</o:p></span></font></p>

<p class=MsoNormal><font size=3 color=black face="Times New Roman"><span
lang=EN-US style='font-size:12.0pt'><o:p>&nbsp;</o:p></span></font></p>

</div>

</body>

</html>

_______________________________________________
Filesystem-dev mailing list
Filesystem-dev@helixcommunity.org
http://lists.helixcommunity.org/mailman/listinfo/filesystem-dev

--===============1888164943==--

[prev in list] [next in list] [prev in thread] [next in thread] 